[InlineCost] Simplify the cold attribute handling in inline-cost.

[chandlerc]

Summary

The inline cost analysis used the cold attribute but only when
ProfileSummaryInfo was available even though that attribute isn't part
of the profile info for the module and is independently available.

This patch lifts the check up into the generic cold callsite detection
logic as for *cold* functions, we routinely treat *all* call sites as
cold. In fact, we shape the probabilities and frequencies this way if
there is any CFG, but sometimes (such as single-basic-block functions)
there isn't any CFG that can exhibit this. Moreover, it seems much
simpler and direct to just query for the cold attribute and be done with
it, skipping all of the probability queries.

This also simplifies what is necessary to write a test using the cold
attribute as now you don't have to remember to run the
profile-summary-info analysis in the new PM. I spent quite some time
confused because I didn't have that analysis around.

Also remove the redundant thresholds between the two ways of detecting
cold calls. Originally there was some concern that we would want to tune
these thresholds independently but so far no such need has arisen and it
is easy enough to add back in. I'd rather have the simpler system in the
interim that is also easier to understand and reason about.

This should be essentially a no-op for most users of LLVM, but will
result in an API simplification and a test simplification.

[davidxl]

lib/Analysis/InlineCost.cpp

659

IIRC, static branch prediction already handles this case. Do you see missing cases?

[chandlerc]

lib/Analysis/InlineCost.cpp

659

Sorry, I tried to explain in my top-level description -- there are cases where despite being cold, the call site won't be cold relative to the function entry. For example, when the call site is *in* the function entry block.

Maybe this still isn't making sense? I can add an example bit of code.

[davidxl]

lib/Analysis/InlineCost.cpp

659

There is an inconsistency between cold callsite relative freq and the cold branch probability --- it is 6% vs 2%. I think we should fix one of them -- IMO, it is more reasonable to fix the static branch prediction.

[chandlerc]

lib/Analysis/InlineCost.cpp

659

Sure? I don't have strong feelings about that. But it seems like that is orthogonal? (Or am I missing something?)

Before this patch, if the branch probability check below failed, we would continue onward and ask PSI and it would still say the call is cold based on this attribute. (I think...)

[davidxl]

lib/Analysis/InlineCost.cpp

659

See my comment to the other patch. My point is what we should not rely this special hand shaking to do this. The generic attribute propagation should already kick in to mark certain callers of a cold callee as cold, and the BFI of the caller's caller should be already good enough to detect this.

[chandlerc]

lib/Analysis/InlineCost.cpp

659

Ok, let's discuss that on that patch.

This patch is *just* trying to cleanup the APIs to make them easier to understand and write tests against. It's just simplifying things and doesn't introduce any significant changes to behavior (that I'm aware of).... Regardless of what we do with the other patch I think this makes the code better?

[davidxl]

lib/Analysis/InlineCost.cpp

659

What you said makes sense in some way. However it feels wrong. If we allow the special handling of cold attribute here, what prevents other special logics from being duplicated here as well --- for instance, EH code, unreachable code, etc .. ?

[chandlerc]

lib/Analysis/InlineCost.cpp

659

All of those have control flow around them which would exhibit the coldness in the BFI analysis, so I don't know why would need them...

The key to me is that it may be useful to change inlining behavior of a cold call even if there is no control flow guarding that callsite.

But all of this seems a discussion around reconsidering the *original* introduction of the logic to check the cold attribute in the ProfileSummaryInfo analysis that was added in r271728 which you reviewed in D20648. I'm fine if you want to revisit the decision to check this attribute, but this patch just moves it around, so I don't know why we should block a cleanup on that...

[eraman]

LGTM. Not handling the cold attribute was an oversight and when the callee is marked cold, it makes sense to treat all its callsites as cold (therefore reasonable to handle in isColdCallsite). I also agree with collapsing the two similar params with the same value until we see a need to have different default values for them.

[davidxl]

convinced.

lgtm

include/llvm/Analysis/InlineCost.h

 
 struct InlineParams {
   /// The default threshold to start with for a callee.
   int DefaultThreshold;
 
   /// Threshold to use for callees with inline hint.
   Optional<int> HintThreshold;
 
-  /// Threshold to use for cold callees.
+  /// Threshold to use for cold call sites and callees.
   Optional<int> ColdThreshold;
 
   /// Threshold to use when the caller is optimized for size.
   Optional<int> OptSizeThreshold;
 
   /// Threshold to use when the caller is optimized for minsize.
   Optional<int> OptMinSizeThreshold;
 
   /// Threshold to use when the callsite is considered hot.
   Optional<int> HotCallSiteThreshold;
 
   /// Threshold to use when the callsite is considered hot relative to function
   /// entry.
   Optional<int> LocallyHotCallSiteThreshold;
-
-  /// Threshold to use when the callsite is considered cold.
-  Optional<int> ColdCallSiteThreshold;
 };
 
 /// Generate the parameters to tune the inline cost analysis based only on the
 /// commandline options.
 InlineParams getInlineParams();
 
 /// Generate the parameters to tune the inline cost analysis based on command
 /// line options. If -inline-threshold option is not explicitly passed,

lib/Analysis/InlineCost.cpp

 static cl::opt<int> InlineThreshold(
     "inline-threshold", cl::Hidden, cl::init(225), cl::ZeroOrMore,
     cl::desc("Control the amount of inlining to perform (default = 225)"));
 
 static cl::opt<int> HintThreshold(
     "inlinehint-threshold", cl::Hidden, cl::init(325),
     cl::desc("Threshold for inlining functions with inline hint"));
 
-static cl::opt<int>
-    ColdCallSiteThreshold("inline-cold-callsite-threshold", cl::Hidden,
-                          cl::init(45),
-                          cl::desc("Threshold for inlining cold callsites"));
-
 // We introduce this threshold to help performance of instrumentation based
 // PGO before we actually hook up inliner with analysis passes such as BPI and
 // BFI.
 static cl::opt<int> ColdThreshold(
     "inlinecold-threshold", cl::Hidden, cl::init(45),
     cl::desc("Threshold for inlining functions with cold attribute"));
 
 static cl::opt<int>
class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
   /// passed to support analyzing indirect calls whose target is inferred by
   /// analysis.
   void updateThreshold(CallSite CS, Function &Callee);
 
   /// Return true if size growth is allowed when inlining the callee at CS.
   bool allowSizeGrowth(CallSite CS);
 
   /// Return true if \p CS is a cold callsite.
-  bool isColdCallSite(CallSite CS, BlockFrequencyInfo *CallerBFI);
+  bool isColdCallSite(CallSite CS, BlockFrequencyInfo *CallerBFI,
+                      Function &Callee);
 
   /// Return a higher threshold if \p CS is a hot callsite.
   Optional<int> getHotCallSiteThreshold(CallSite CS,
                                         BlockFrequencyInfo *CallerBFI);
 
   // Custom analysis routines.
   bool analyzeBlock(BasicBlock *BB, SmallPtrSetImpl<const Value *> &EphValues);
 
if (InvokeInst *II = dyn_cast<InvokeInst>(Instr)) {
     if (isa<UnreachableInst>(II->getNormalDest()->getTerminator()))
       return false;
   } else if (isa<UnreachableInst>(Instr->getParent()->getTerminator()))
     return false;
 
   return true;
 }
 
-bool CallAnalyzer::isColdCallSite(CallSite CS, BlockFrequencyInfo *CallerBFI) {
+bool CallAnalyzer::isColdCallSite(CallSite CS, BlockFrequencyInfo *CallerBFI,
+                                  Function &Callee) {
+  // Simplest test is for the cold attribute on the function itself. These
+  // should *always* be considered cold.
+  if (Callee.hasFnAttribute(Attribute::Cold))
+    return true;
+
   // If global profile summary is available, then callsite's coldness is
   // determined based on that.
   if (PSI && PSI->hasProfileSummary())
     return PSI->isColdCallSite(CS, CallerBFI);
 
   // Otherwise we need BFI to be available.
   if (!CallerBFI)
     return false;
if (!Caller->optForMinSize()) {
     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)) {
+    } else if (isColdCallSite(CS, CallerBFI, Callee)) {
       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);
+      Threshold = MinIfValid(Threshold, Params.ColdThreshold);
     } 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.
InlineParams llvm::getInlineParams(int Threshold) {
   // Params.LocallyHotCallSiteThreshold from -locally-hot-callsite-threshold if
   // we know that optimization level is O3 (in the getInlineParams variant that
   // takes the opt and size levels).
   // FIXME: Remove this check (and make the assignment unconditional) after
   // addressing size regression issues at O2.
   if (LocallyHotCallSiteThreshold.getNumOccurrences() > 0)
     Params.LocallyHotCallSiteThreshold = LocallyHotCallSiteThreshold;
 
-  // Set the ColdCallSiteThreshold knob from the -inline-cold-callsite-threshold.
-  Params.ColdCallSiteThreshold = ColdCallSiteThreshold;
-
   // Set the OptMinSizeThreshold and OptSizeThreshold params only if the
   // -inlinehint-threshold commandline option is not explicitly given. If that
   // option is present, then its value applies even for callees with size and
   // minsize attributes.
   // If the -inline-threshold is not specified, set the ColdThreshold from the
   // -inlinecold-threshold even if it is not explicitly passed. If
   // -inline-threshold is specified, then -inlinecold-threshold needs to be
   // explicitly specified to set the ColdThreshold knob

test/Transforms/Inline/bfi-update.ll

-; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -S -inline-threshold=50 -inline-cold-callsite-threshold=0 -hot-callsite-threshold=50 | FileCheck %s
+; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -S -inline-threshold=50 -inlinecold-threshold=0 -hot-callsite-threshold=50 | FileCheck %s
 ; This tests incremental updates to caller's BFI as a callee gets inlined.
 ; In bottom-up inlining, first c->e inlining is considered and fails because
 ; e's size exceeds the threshold of 50. Then a->c inlining is considered and it
 ; succeeds. a's BFI is updated incrementally. As c's blocks get pruned, the 
 ; block with label cond_false is removed and since the remanining code is
 ; straight-line a single block gets cloned into a. This block should get the
 ; maximum block frequency among the original blocks in c. If it gets the
 ; frequency of the block with label cond_true in @c, its frequency will be

test/Transforms/Inline/inline-cold-callsite-pgo.ll

-; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -inline-threshold=100 -inline-cold-callsite-threshold=0 -S | FileCheck %s
+; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -inline-threshold=100 -inlinecold-threshold=0 -S | FileCheck %s
 
 ; This tests that a cold callsite gets the inline-cold-callsite-threshold
 ; and does not get inlined. Another callsite to an identical callee that
 ; is not cold gets inlined because cost is below the inline-threshold.
 
 define i32 @callee1(i32 %x) !prof !21 {
   %x1 = add i32 %x, 1
   %x2 = add i32 %x1, 1

test/Transforms/Inline/inline-cold-callsite.ll

-
-; RUN: opt < %s -passes='require<profile-summary>,cgscc(inline)' -inline-threshold=100 -inline-cold-callsite-threshold=0 -S | FileCheck %s
+; RUN: opt < %s -passes='cgscc(inline)' -inline-threshold=100 -inlinecold-threshold=0 -S | FileCheck %s
 
 ; This tests that a cold callsite gets the inline-cold-callsite-threshold
 ; and does not get inlined. Another callsite to an identical callee that
 ; is not cold gets inlined because cost is below the inline-threshold.
 
 define void @callee() {
   call void @extern()
   call void @extern()

test/Transforms/Inline/pr26698.ll

-; RUN: opt -S -inline -inline-threshold=100 -inline-cold-callsite-threshold=100 < %s | FileCheck %s
-; RUN: opt -S -passes='cgscc(inline)' -inline-threshold=100 -inline-cold-callsite-threshold=100 < %s | FileCheck %s
+; RUN: opt -S -inline -inline-threshold=100 -inlinecold-threshold=100 < %s | FileCheck %s
+; RUN: opt -S -passes='cgscc(inline)' -inline-threshold=100 -inlinecold-threshold=100 < %s | FileCheck %s
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
 target triple = "i686-pc-windows-msvc18.0.0"
 
 declare void @g(i32)
 
 define void @f() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @g(i32 0)