[PATCH] D28369: Refactor inline threshold update code.

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

In https://reviews.llvm.org/D28369#638452, @chandlerc wrote:

> In https://reviews.llvm.org/D28369#638408, @eraman wrote:
>
> > In https://reviews.llvm.org/D28369#638393, @davidxl wrote:
> >
> > > Should we honor optForMinSize for HotCallsite case as well?
> >
> >
> > Yes, unless and until someone tunes PGO + -Os case.
>
>
> This is the '-Oz' case, where PGO doesn't really matter, so I suspect this will always be structured this way.


Right. It probably doesn't make sense to use the high threshold used for hot callee even with -Os, but that'll of course have to be a separate patch.

> 
> 
>>> I also suggest split the source hint based check from the profile driven check.  Also check hotcallsite first:
>>> 
>>> if (!Caller->optForMinSize() ) {
>>> 
>>>   if (...InlineHint)
>>>     Threshold = MaxIfValid (...);
>>>    
>>>   if (HotCallsite)
>>>      ...
>>>   else if (HotCallee) 
>>>     ..
>>> 
>>> }
>> 
>> Note that HotCallsite and HotCallee are mutually exclusive and if the callsite is hot that takes precedence over HotCallee. So 
>>  your comment on "check hot callsite first" is already done.  I also don't see much point in writing
>> 
>> if (Callee.hasFnAttribute(Attribute::InlineHint))
>> 
>>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>> 
>> if ( HotCallee)
>> 
>>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>>    
>> 
>> instead of the current
>> 
>> if (Callee.hasFnAttribute(Attribute::InlineHint) || HotCallee)
>> 
>>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
> 
> But I think this whole thing can be written much more simply, and with fewer variables and correlated conditions:
> 
>   // Listen to the inlinehint attribute or profile based hotness information
>   // when it would increase the threshold and the caller does not need to
>   // minimize its size.
>   if (!Caller->optForMinSize()) {
>     if (Callee.hasFnAttribute(Attribute::InlineHint) || HotCallee)
>       Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>    
>     if (PSI) {
>       uint64_t TotalWeight;
>       if (CS.getInstruction()->extractProfTotalWeight(TotalWeight) &&
>           PSI->isHotCount(TotalWeight)) {
>       Threshold = MaxIfValid(Threshold, Params.HotCallSiteThreshold);
>     } else if (PSI->isFunctionEntryHot(&Callee)) {
>       // If callsite hotness can not be determined, we may still know
>       // that the callee is hot and can use a smaller hint.
>       Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>     } else if (PSI->isFunctionEntryCold(&Callee)) {
>       Threshold = MinIfValid(Threshold, Params.ColdThreshold);
>     }
>   }
>    

Thanks. Rewrote as you suggested with a  minor comment tweak.

> This does remove the cold profile data from reducing the threshold in '-Oz' but I honestly can't understand why we would ever want a threshold *lower* than '-Oz'... at that level, we're only inlining when it actively shrinks code, and even for a cold call we would want to do that?

Makes sense.


https://reviews.llvm.org/D28369