[PATCH] D28369: Refactor inline threshold update code.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

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.

>> 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);
    }
  }

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?


https://reviews.llvm.org/D28369