Re: RFC: improving estimate_num_insns

[Josh Conner <jconner@apple.com>]

On Jul 12, 2005, at 1:07 PM, Steven Bosscher wrote:

> You don't say what compiler you used for these measurements.  I  
> suppose
> you used mainline?

Yes, I am working with the mainline.

> I think you should look at a lot more code than this.

OK - I stopped because I was seeing fairly consistent results.   
However, since both of these files were from the same source base, I  
can see how this may not be representative.  I'll try some more  
examples from different source code, including C++.

>> Thinking that there may be room for improvement on this, I tried this
>> same experiment with a couple of adjustments to estimate_num_insns:
>> - Instead of ignoring constants, assign them a weight of 2
>> instructions (one for the constant itself and one to load into  
>> memory)
>>
>
> Why the load into memory, you mean constants that must be loaded from
> the constant pool?  I guess the cost for the constant itself  
> depends on
> whether it is a legitimate constant or not, and how it is loaded, and
> this is all very target specific.  But a cost greater than 0 probably
> makes sense.

Good point - I was thinking of constant pools when I wrote that, even  
though that isn't always the case.

> It would be nice if you retain the comment about constants in the
> existing code somehow.  The cost of constants is not trivial, and you
> should explain your choice better in a comment.

I'm not sure which comment you mean - the one from my email, or the  
one that was originally in the source code?

>> - Instead of ignoring case labels, assign them a weight of 2
>> instructions (to represent the cost of control logic to get there)
>>
>
> This depends on how the switch statement is expanded, e.g. to a binary
> decision tree, or to a table jump, or to something smaller than that.
> So again (like everything else, sadly) this is highly target-specific
> and even context-specific.  I'd think a cost of 2 is too  
> pessimistic in
> most cases.

Do you mean that 2 is too high?  I actually got (slightly) better  
statistical results with a cost of 3, but I had the same reaction  
(that it was too pessimistic), which is why I settled on 2.

> You could look at the code in stmt.c to see how switch statements are
> expanded.  Maybe there is a cheap test you can do to make CASE_LABELs
> free for very small switch statements (i.e. the ones which should  
> never
> hold you back from inlining the function containing them ;-).

I'll look into this.

>> For what it's worth, code size is equal to or smaller for all
>> benchmarks across all platforms.
>>
>
> What about the compile time?

Oh no!  I didn't measure this.  I will have a look.

>> So, here are the open issues I see at this point:
>> 1. It appears that this change to estimate_num_instructions generates
>> a much better estimate of actual code size.  However, the benchmark
>> results are ambiguous.  Is this patch worth considering as-is?
>>
>
> I would say you'd at least have to look into the ppc gzip and eon
> regressions before this is acceptable.  But it is not my decision to
> make, of course.

Makes sense.  I'll see what kind of time I can put into this.

>> 2. Increasing instruction weights causes the instruction-based values
>> (e.g., --param max-inline-insns-auto) to be effectively lower.
>> However, changing these constants/defaults as in the second patch
>> will cause a semantic change to anyone who is setting these values at
>> the command line.  Is that change acceptable?
>>
>
> This has constantly changed from one release to the next since GCC  
> 3.3,
> so I don't think this should be a problem.

Whew...

>> Thoughts?  Advice?
>>
>
> ...on to advice then.
>
> First of all, I think you should handle ARRAY_RANGE_REF and ARRAY_REF
> the same.  And you probably should make BIT_FIELD_REF more expensive,
> and maybe make its cost depend on which bit you're addressing (you can
> see that in operands 1 and 2 of the BIT_FIELD_REF).  Its current cost
> of just 1 is probably too optimistic, just like the other cases you  
> are
> trying to address with this patch.

Great - thanks for the suggestions, I'll try these changes as well  
and see if I get even better correlation.

> Second, you may want to add a target hook to return the cost of target
> builtins.  Even builtins that expand to just one instruction are now
> counted as 16 insns plus the cost of the arguments list.  This badly
> hurts when you use e.g. SSE intrinsics.  It's probably not an issue  
> for
> the benchmark you looked at now, but maybe you want to look into it
> anyway, while you're at it.

OK, I'll look into this.

> Third,
>
>> (measured at -O3 with inlining disabled):
>>
> Then why not just -O2 with inlining disabled?  Now you have enabled
> loop unswitching, which is known to sometimes significantly increase
> code size.

Well, I thought that inlining estimates were most likely to be  
relevant at O3, and so I should include as many other optimizations  
as were likely to be performed alongside inlining, even though this  
may make it more difficult to get accurate estimates.

> Fourth, look at _much_ more code than this.  I would especially  
> look at
> a lot more C++ code, which is where our inliner heuristics can really
> dramatically improve or destroy performance.

OK.

>   See Richard Guenther's
> inline heuristics tweaks from earlier this year, in the thread  
> starting
> here: http://gcc.gnu.org/ml/gcc-patches/2005-03/msg01936.html.

I remember reading this when it was first posted, but looking at it  
again after my experiments I see the additional relevance.   
Especially the decision to assign a function call a cost of 16, which  
I quickly identified as hurting our size estimates, but which also  
improves inlining performance!

> Finally, you've apparently used grep to find all the places where
> PARAM_MAX_INLINE_INSNS_SINGLE and its friends are used, but you hve
> missed the ones in opts.c and maybe elsewhere.

Hmmm - I looked for all of the places where estimate_num_insns was  
called.  I still don't see anything in opts.c -- can you give me a  
little more of a hint?

> Good luck, and thanks for working on this difficult stuff.

Thank you for the helpful comments!

- Josh