On 06/06/2012 02:29 AM, Richard Guenther wrote:
Pre-computing and caching things is to avoid creating RTXen over and over.
As you have discarded this completely did you try to measure the cost
of doing so in terms of produced garbage and compile-time cost? Did you
consider changing the target interface of IVOPTs to a (bunch of) new
target hooks that avoid the RTX generation (which in fact we are not sure
that we'll end up producing anyways in exactly that form due to subsequent
optimizations)?
Since there seemed to be resistance to removing the pre-computing of
costs, I've spent much of the last week trying to glue fixes on the
existing code while preserving the caching, and just am not happy with
the result. It makes the code too complicated, adds additional overhead
by precomputing more things, and still does not fix the lurking bugs WRT
differing costs for different values of constant offsets and the like.
Basically, I don't want to put my name on anything that ugly. :-P So,
I went back and did some compile time benchmarking on my previously
posted patch instead.
I used the bzip2 and gcc test programs available here:
http://people.csail.mit.edu/smcc/projects/single-file-programs/
These are respectively large and gigantic single-file programs, so you
would expect the performance effects of caching to be particularly
evident here as there would likely be many loops involving the same
modes in each compilation unit. I compiled them using a native x86_64
build with "time gcc -c -O3", and ran each set of timings 3 times with
an unmodified build and with my previously-posted patch. And, it turns
out there is no obvious difference in the results.
bzip2 base: 5.82, 5.82, 5.75
bzip2 patched: 5.73, 5.71, 5.85
gcc base: 4m44.390, 4m44.270, 4m44.060
gcc patched: 4m44.210, 4m44.530, 4m44.040
Can you split the patch into pieces fixing the above bugs separately?
Removing the pre-compute and caching is the most questionable change,
the others look like real bugs (the symbol cost might be questionable as
well).
Given that most of the bugs were in the same function and were fixed by
rewriting it completely, trying to split up the patch seems kind of
pointless, to me.
CC-ing Zdenek for his opinions (disclaimer: I didn't look at the actual patch).
Might somebody be willing to review the patch as posted?
FWIW, I was doing some digging around in the mail archives to see if
there was any discussion that would help me understand the rationale for
the current cost model better. What I found was that when the ivopts
pass was originally added back in 2004, the costs computation was one of
the things that was specifically mentioned as needing work at least to
document it better, but the patch was rushed through and approved
without any thorough review because the Stage 3 deadline was looming.
Anyway, given that get_address_cost has had a big FIXME on it all these
years, it seems to me like maybe it's time to try to fix it?
-Sandra
