Refactor inline threshold update code.

[eraman]

Summary

Functional change: Previously, if a callee is cold, we used ColdThreshold if it minimizes the existing threshold. This was irrespective of whether we were optimizing for minsize (-Oz) or not. But -Oz uses very low threshold to begin with and the inlining with -Oz is expected to be tuned for lowering code size, so there is no good reason to set an even lower threshold for cold callees. We now lower the threshold for cold callees only when -Oz is not used. For default values of -inlinethreshold and -inlinecold-threshold, this change has no effect and this simplifies the code.

NFC changes: Group all threshold updates that are guarded by !Caller->optForMinSize() and within that group threshold updates that require profile summary info.

[davidxl]

There is a slight difference here. In the old code, for hot callsite to hot callee, the threshold is max of (origthreshold, HintThreshold, HotCallSiteThreshold). With the new change, it becomes max (original_threshold, HotCallSiteThreshold).

[eraman]

In D28369#638375, @davidxl wrote:

There is a slight difference here. In the old code, for hot callsite to hot callee, the threshold is max of (origthreshold, HintThreshold, HotCallSiteThreshold). With the new change, it becomes max (original_threshold, HotCallSiteThreshold).

Good catch! But I'll leave the patch as it is (except the NFC in the title) since the whole point is to move away from using hotness of callee to hotness of callsite. Also, this will have no impact with the default values since HotCallSiteThreshold >> HintThreshold.

[davidxl]

Should we honor optForMinSize for HotCallsite case as well?

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) 
  ..

}

[eraman]

In D28369#638393, @davidxl wrote:

Should we honor optForMinSize for HotCallsite case as well?

Yes, unless and until someone tunes PGO + -Os case.

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

[chandlerc]

In D28369#638408, @eraman wrote:

In 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?

[chandlerc]

(obviously, my code had a bug, don't test for HotCallee when doing the inlinehint one)

Also, it might make sense to make the hot callee tunable independently from the inlinehint attribute even if the initial value is exactly the same.

[eraman]

Rewrite as per Chandler's suggestion

[eraman]

In D28369#638452, @chandlerc wrote:

In D28369#638408, @eraman wrote:

In 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.

[davidxl]

lgtm

[chandlerc]

This LGTM, but please make sure you update the changed escription to explain exactly what all is changing here. For example with the cold vs. minsize, I think it's safe, and we can't even really usefully test it as in tree this never happens. But you may need to explain to people that if they *do* have a crazy minsize vs. cold threshold, they may need to fix them now.

Thanks for the refactoring!

[eraman]

In D28369#639863, @chandlerc wrote:

This LGTM, but please make sure you update the changed escription to explain exactly what all is changing here. For example with the cold vs. minsize, I think it's safe, and we can't even really usefully test it as in tree this never happens. But you may need to explain to people that if they *do* have a crazy minsize vs. cold threshold, they may need to fix them now.

Thanks for the refactoring!

Thanks. I've updated the description.

llvm/trunk/lib/Analysis/InlineCost.cpp

void CallAnalyzer::updateThreshold(CallSite CS, Function &Callee) {
 
   // Use the OptMinSizeThreshold or OptSizeThreshold knob if they are available
   // and reduce the threshold if the caller has the necessary attribute.
   if (Caller->optForMinSize())
     Threshold = MinIfValid(Threshold, Params.OptMinSizeThreshold);
   else if (Caller->optForSize())
     Threshold = MinIfValid(Threshold, Params.OptSizeThreshold);
 
-  bool HotCallsite = false;
+  // Adjust the threshold based on inlinehint attribute and profile based
+  // hotness information if the caller does not have MinSize attribute.
+  if (!Caller->optForMinSize()) {
+    if (Callee.hasFnAttribute(Attribute::InlineHint))
+      Threshold = MaxIfValid(Threshold, Params.HintThreshold);
+    if (PSI) {
       uint64_t TotalWeight;
-  if (PSI && CS.getInstruction()->extractProfTotalWeight(TotalWeight) &&
+      if (CS.getInstruction()->extractProfTotalWeight(TotalWeight) &&
           PSI->isHotCount(TotalWeight)) {
-    HotCallsite = true;
-  }
-
-  // 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.
-  bool InlineHint = Callee.hasFnAttribute(Attribute::InlineHint) ||
-                    (PSI && PSI->isFunctionEntryHot(&Callee));
-  if (InlineHint && !Caller->optForMinSize())
-    Threshold = MaxIfValid(Threshold, Params.HintThreshold);
-
-  if (HotCallsite && !Caller->optForMinSize())
         Threshold = MaxIfValid(Threshold, Params.HotCallSiteThreshold);
-
-  bool ColdCallee = PSI && PSI->isFunctionEntryCold(&Callee);
-  // For cold callees, use the ColdThreshold knob if it is available and reduces
-  // the threshold.
-  if (ColdCallee)
+      } else if (PSI->isFunctionEntryHot(&Callee)) {
+        // If callsite hotness can not be determined, we may still know
+        // that the callee is hot and treat it as a weaker hint for threshold
+        // increase.
+        Threshold = MaxIfValid(Threshold, Params.HintThreshold);
+      } else if (PSI->isFunctionEntryCold(&Callee)) {
         Threshold = MinIfValid(Threshold, Params.ColdThreshold);
+      }
+    }
+  }
 
   // Finally, take the target-specific inlining threshold multiplier into
   // account.
   Threshold *= TTI.getInliningThresholdMultiplier();
 }
 
 bool CallAnalyzer::visitCmpInst(CmpInst &I) {
   Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);