[InlineCost] Don't return early when allowSizeGrowth(CS) is false

[paquette]

Summary

This patch concerns chains of calls like so:

pluto();

foo() {
  pluto();
}

wibble() {
  foo();
}

Suppose foo has internal linkage. Also let's say that the call to foo within wibble is the only call to foo within the module.

Normally, the inlining cost model will apply a large bonus to ensure that foo is inlined into wibble because there's basically no cost in doing so. However, if wibble is unreachable-terminated, this won't happen. This is because the cost model currently does the following:

1. Check if the block containing the call is unreachable-terminated (allowSizeGrowth)
2. If it is, set the threshold to 0 and return

This happens before the bonus is applied. Therefore, any "zero-cost" case relying on the bonus won't ever be inlined when we're dealing with unreachable-terminated blocks.

This commit

• Removes the early return when allowSizeGrowth is false
• Wraps the threshold tweaks in a conditional which is true only when size growth is allowed.

The tweaks are wrapped in a conditional to reflect that we only want to inline when the cost of inlining is truly 0 or better; any modifications to the threshold would break this assertion. The early return is removed to facilitate inlining the example case.

This produced some minor code size improvements for ARM, AArch64, and x86-64 at Oz.

Output from compare.py here for Oz: https://hastebin.com/fojuquzoru.erl

Edit: More clarity. The word salad from before wasn't that great.

[eraman]

lib/Analysis/InlineCost.cpp

845

Move the code applying last-call-to-static bonus to the top. Then, you could early exit after setting the threshold to 0 under the !allowSizeGrowth condition.

[paquette]

Update patch to move the LastCallToStaticBonus logic to the top of updateThreshold.

[fhahn]

lib/Analysis/InlineCost.cpp

847

The changes after this point do not seem to be necessary, i.e. could they stay at the same place?

[fhahn]

Jessica, are you still planing on pushing this change? :)

[paquette]

Wow, I entirely forgot about this somehow.

I'll take a look and see if it's still relevant...

[fhahn]

Is this still needed? Marking as changes requested to clear from review queue.

lib/Analysis/InlineCost.cpp

CallAnalyzer::getHotCallSiteThreshold(CallSite CS,
   if (CallSiteFreq >= CallerEntryFreq * HotCallSiteRelFreq)
     return Params.LocallyHotCallSiteThreshold;
 
   // Otherwise treat it normally.
   return None;
 }
 
 void CallAnalyzer::updateThreshold(CallSite CS, Function &Callee) {
-  // If no size growth is allowed for this inlining, set Threshold to 0.
-  if (!allowSizeGrowth(CS)) {
+  // If no size growth is allowed in this round of inlining, set the threshold
+  // to 0. We only want to inline if it has 0 cost or better in this case.
+  bool AllowSizeGrowth = allowSizeGrowth(CS);
+  if (!AllowSizeGrowth)
     Threshold = 0;
-    return;
-  }
 
   Function *Caller = CS.getCaller();
 
   // return min(A, B) if B is valid.
   auto MinIfValid = [](int A, Optional<int> B) {
     return B ? std::min(A, B.getValue()) : A;
   };
 
void CallAnalyzer::updateThreshold(CallSite CS, Function &Callee) {
 
   // Lambda to set all the above bonus and bonus percentages to 0.
   auto DisallowAllBonuses = [&]() {
     SingleBBBonusPercent = 0;
     VectorBonusPercent = 0;
     LastCallToStaticBonus = 0;
   };
 
-  // Use the OptMinSizeThreshold or OptSizeThreshold knob if they are available
-  // and reduce the threshold if the caller has the necessary attribute.
+  // If we're allowed to increase size, then bump the threshold up and down
+  // based off properties of the caller. Otherwise, skip ahead and finalize the
+  // cost and threshold.
+  if (AllowSizeGrowth) {
+    // 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);
       // For minsize, we want to disable the single BB bonus and the vector
-    // bonuses, but not the last-call-to-static bonus. Inlining the last call to
-    // a static function will, at the minimum, eliminate the parameter setup and
-    // call/return instructions.
+      // bonuses, but not the last-call-to-static bonus. Inlining the last call
+      // to a static function will, at the minimum, eliminate the parameter
+      // setup and call/return instructions.
       SingleBBBonusPercent = 0;
       VectorBonusPercent = 0;
     } else if (Caller->optForSize())
       Threshold = MinIfValid(Threshold, Params.OptSizeThreshold);
 
     // 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);
 
       // FIXME: After switching to the new passmanager, simplify the logic below
       // by checking only the callsite hotness/coldness as we will reliably
       // have local profile information.
       //
       // Callsite hotness and coldness can be determined if sample profile is
       // used (which adds hotness metadata to calls) or if caller's
       // BlockFrequencyInfo is available.
       BlockFrequencyInfo *CallerBFI = GetBFI ? &((*GetBFI)(*Caller)) : nullptr;
       auto HotCallSiteThreshold = getHotCallSiteThreshold(CS, CallerBFI);
       if (!Caller->optForSize() && HotCallSiteThreshold) {
         DEBUG(dbgs() << "Hot callsite.\n");
         // FIXME: This should update the threshold only if it exceeds the
         // current threshold, but AutoFDO + ThinLTO currently relies on this
         // behavior to prevent inlining of hot callsites during ThinLTO
         // compile phase.
         Threshold = HotCallSiteThreshold.getValue();
       } else if (isColdCallSite(CS, CallerBFI)) {
         DEBUG(dbgs() << "Cold callsite.\n");
         // Do not apply bonuses for a cold callsite including the
         // LastCallToStatic bonus. While this bonus might result in code size
         // reduction, it can cause the size of a non-cold caller to increase
         // preventing it from being inlined.
         DisallowAllBonuses();
         Threshold = MinIfValid(Threshold, Params.ColdCallSiteThreshold);
       } else if (PSI) {
         // Use callee's global profile information only if we have no way of
         // determining this via callsite information.
         if (PSI->isFunctionEntryHot(&Callee)) {
           DEBUG(dbgs() << "Hot callee.\n");
           // 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)) {
           DEBUG(dbgs() << "Cold callee.\n");
           // Do not apply bonuses for a cold callee including the
           // LastCallToStatic bonus. While this bonus might result in code size
           // reduction, it can cause the size of a non-cold caller to increase
           // preventing it from being inlined.
           DisallowAllBonuses();
           Threshold = MinIfValid(Threshold, Params.ColdThreshold);
         }
       }
     }
+  }
 
   // Finally, take the target-specific inlining threshold multiplier into
   // account.
+  // Note that Threshold = 0 when size growth is not allowed.
   Threshold *= TTI.getInliningThresholdMultiplier();
 
   SingleBBBonus = Threshold * SingleBBBonusPercent / 100;
   VectorBonus = Threshold * VectorBonusPercent / 100;
 
   bool OnlyOneCallAndLocalLinkage =
       F.hasLocalLinkage() && F.hasOneUse() && &F == CS.getCalledFunction();
   // If there is only one call of the function, and it has internal linkage,

test/Transforms/Inline/ARM/unreachable.ll

+; RUN: opt < %s -mtriple=arm--- -S -inline | FileCheck %s
+
+declare i32* @pluto() local_unnamed_addr #0
+
+define internal void @foo() local_unnamed_addr #0 {
+bb:
+  call i32* @pluto() #0
+  ret void
+}
+
+define void @wibble() unnamed_addr #0 {
+; CHECK-LABEL: wibble
+; CHECK-NOT: @foo()
+; CHECK: %0 = call i32* @pluto()
+; CHECK-NEXT: unreachable
+bb:
+  tail call void @foo() #0
+  unreachable
+}
+
+attributes #0 = { minsize optsize }