[Inliner] Attribute callsites with inline remarks

[yrouban]

Summary

Sometimes reading an output *.ll file it is not easy to understand why some callsites are not inlined. We can read output of inline remarks (option --pass-remarks-missed=inline) and try correlating its messages with the callsites.

An easier way proposed by this patch is to add to every callsite processed by Inliner an attribute with the latest message that describes the cause of not inlining this callsite. The attribute is called inline-remark. By default this feature is off. It can be switched on by the option -inline-remark-attribute.

For example in the provided test the result method @test1 has two callsites @bar and inline remarks report different inlining missed reasons:

remark: <unknown>:0:0: bar not inlined into test1 because too costly to inline (cost=-5, threshold=-6)
remark: <unknown>:0:0: bar not inlined into test1 because it should never be inlined (cost=never): recursive

It is not clear which remark correspond to which callsite. With the inline remark attribute enabled we get the reasons attached to their callsites:

define void @test1() {
  call void @bar(i1 true) #0
  call void @bar(i1 false) #2
  ret void
}
attributes #0 = { "inline-remark"="(cost=-5, threshold=-6)" }
..
attributes #2 = { "inline-remark"="(cost=never): recursive" }

Patch by: yrouban (Yevgeny Rouban)

[xbolva00]

test/Transforms/Inline/inline-remark.ll

1 ↗

(On Diff #159676)

Fix this line please (spaces..)

[yrouban]

fixed the test: removed extra whitespaces, changed description of Test1

[xbolva00]

looks good but wait also for @tejohnson 's approval

[tejohnson]

A few comments/questions.

lib/Transforms/IPO/Inliner.cpp

119 ↗

(On Diff #160267)

Add a cl::desc for option.

507 ↗

(On Diff #160267)

LLVM coding style prefers early return to code nested in if body.

646 ↗

(On Diff #160267)

The comment here isn't very meaningful. Where does the OIC explain this? Please expand/replace comment (here and at line 1034).

673 ↗

(On Diff #160267)

I'm not sure what this ends up looking like - can you add a case with this string to your test?

1053 ↗

(On Diff #160267)

Why IR.message here but only IR online 673?

[yrouban]

I will address all comments by the end of the next week.

[yrouban]

lib/Transforms/IPO/Inliner.cpp

673 ↗

(On Diff #160267)

You are right, I forgot to change the return type of InlineCallIfPossible() from bool to InlineResult.

[yrouban]

• added cl:desc to inline-remark-attribute option
• changed return type of InlineCallIfPossible() from bool to InlineResult
• added one more test to see InlineResult printed in inline-remark attribute
• reformatted body of function setInlineRemark()

[xbolva00]

ninja check-all? :)

[yrouban]

Re-checking is in progress. I'm almost sure it will pass as inline-remark-attribute is set to false by default.

[yrouban]

make check-all passed.

[xbolva00]

In D50435#1212191, @yrouban wrote:

make check-all passed.

Thanks. Any final words from @tejohnson ?

[tejohnson]

LGTM

[xbolva00]

Build failed

http://lab.llvm.org:8011/builders/lldb-amd64-ninja-freebsd11/builds/30216/steps/ninja%20build%20local/logs/stdio

[yrouban]

Fixed build failure by moving operator<<(std::basic_ostream<char> &R, const ore::NV &Arg) out of #ifndef NDEBUG condition.
Started ninja check-all for non-debug config.

[yrouban]

ninja check-all passed except one test failure Clang :: Modules/prune.m.
It seems to be unrelated to this patch as it is reproducible with clean master sources.

[xbolva00]

In D50435#1214065, @yrouban wrote:

ninja check-all passed except one test failure Clang :: Modules/prune.m.
It seems to be unrelated to this patch as it is reproducible with clean master sources.

Thanks, I will try to recommit it later this day.

[yrouban]

Thank you Dávid for your help.

[xbolva00]

Hi @yrouban,

Since you did some work in this area, can you look at this bug report please?

https://bugs.llvm.org/show_bug.cgi?id=42084

[yrouban]

In D50435#1530337, @xbolva00 wrote:

Hi @yrouban,
Since you did some work in this area, can you look at this bug report please?
https://bugs.llvm.org/show_bug.cgi?id=42084

Interesting. I will try but next week.

llvm/trunk/lib/Transforms/IPO/Inliner.cpp

static cl::opt<InlinerFunctionImportStatsOpts> InlinerFunctionImportStats(
     "inliner-function-import-stats",
     cl::init(InlinerFunctionImportStatsOpts::No),
     cl::values(clEnumValN(InlinerFunctionImportStatsOpts::Basic, "basic",
                           "basic statistics"),
                clEnumValN(InlinerFunctionImportStatsOpts::Verbose, "verbose",
                           "printing of statistics for each inlined function")),
     cl::Hidden, cl::desc("Enable inliner stats for imported functions"));
 
+/// Flag to add inline messages as callsite attributes 'inline-remark'.
+static cl::opt<bool>
+    InlineRemarkAttribute("inline-remark-attribute", cl::init(false),
+                          cl::Hidden,
+                          cl::desc("Enable adding inline-remark attribute to"
+                                   " callsites processed by inliner but decided"
+                                   " to be not inlined"));
+
 LegacyInlinerBase::LegacyInlinerBase(char &ID) : CallGraphSCCPass(ID) {}
 
 LegacyInlinerBase::LegacyInlinerBase(char &ID, bool InsertLifetime)
     : CallGraphSCCPass(ID), InsertLifetime(InsertLifetime) {}
 
 /// For this class, we declare that we require and preserve the call graph.
 /// If the derived class implements this method, it should
 /// always explicitly call the implementation here.

 /// If it is possible to inline the specified call site,
 /// do so and update the CallGraph for this operation.
 ///
 /// This function also does some basic book-keeping to update the IR.  The
 /// InlinedArrayAllocas map keeps track of any allocas that are already
 /// available from other functions inlined into the caller.  If we are able to
 /// inline this call site we attempt to reuse already available allocas or add
 /// any new allocas to the set if not possible.
-static bool InlineCallIfPossible(
+static InlineResult InlineCallIfPossible(
     CallSite CS, InlineFunctionInfo &IFI,
     InlinedArrayAllocasTy &InlinedArrayAllocas, int InlineHistory,
     bool InsertLifetime, function_ref<AAResults &(Function &)> &AARGetter,
     ImportedFunctionsInliningStatistics &ImportedFunctionsStats) {
   Function *Callee = CS.getCalledFunction();
   Function *Caller = CS.getCaller();
 
   AAResults &AAR = AARGetter(*Callee);
if (IC.isAlways()) {
     R << "(cost=" << ore::NV("Cost", IC.getCost())
       << ", threshold=" << ore::NV("Threshold", IC.getThreshold()) << ")";
   }
   if (const char *Reason = IC.getReason())
     R << ": " << ore::NV("Reason", Reason);
   return R;
 }
 
-#ifndef NDEBUG
 static std::string inlineCostStr(const InlineCost &IC) {
   std::stringstream Remark;
   Remark << IC;
   return Remark.str();
 }
-#endif
 
 /// Return the cost only if the inliner should attempt to inline at the given
 /// CallSite. If we return the cost, we will emit an optimisation remark later
 /// using that cost, so we won't do so from this function.
 static Optional<InlineCost>
 shouldInline(CallSite CS, function_ref<InlineCost(CallSite CS)> GetInlineCost,
              OptimizationRemarkEmitter &ORE) {
   using namespace ore;
ORE.emit([&]() {
     bool AlwaysInline = IC.isAlways();
     StringRef RemarkName = AlwaysInline ? "AlwaysInline" : "Inlined";
     return OptimizationRemark(DEBUG_TYPE, RemarkName, DLoc, Block)
            << ore::NV("Callee", &Callee) << " inlined into "
            << ore::NV("Caller", &Caller) << " with " << IC;
   });
 }
 
+static void setInlineRemark(CallSite &CS, StringRef message) {
+  if (!InlineRemarkAttribute)
+    return;
+
+  Attribute attr = Attribute::get(CS->getContext(), "inline-remark", message);
+  CS.addAttribute(AttributeList::FunctionIndex, attr);
+}
+
 static bool
 inlineCallsImpl(CallGraphSCC &SCC, CallGraph &CG,
                 std::function<AssumptionCache &(Function &)> GetAssumptionCache,
                 ProfileSummaryInfo *PSI, TargetLibraryInfo &TLI,
                 bool InsertLifetime,
                 function_ref<InlineCost(CallSite CS)> GetInlineCost,
                 function_ref<AAResults &(Function &)> AARGetter,
                 ImportedFunctionsInliningStatistics &ImportedFunctionsStats) {
for (BasicBlock &BB : *F)
 
         // If this is a direct call to an external function, we can never inline
         // it.  If it is an indirect call, inlining may resolve it to be a
         // direct call, so we keep it.
         if (Function *Callee = CS.getCalledFunction())
           if (Callee->isDeclaration()) {
             using namespace ore;
 
+            setInlineRemark(CS, "unavailable definition");
             ORE.emit([&]() {
               return OptimizationRemarkMissed(DEBUG_TYPE, "NoDefinition", &I)
                      << NV("Callee", Callee) << " will not be inlined into "
                      << NV("Caller", CS.getCaller())
                      << " because its definition is unavailable"
                      << setIsVerbose();
             });
             continue;
for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
       if (!IsTriviallyDead) {
         // If this call site was obtained by inlining another function, verify
         // that the include path for the function did not include the callee
         // itself.  If so, we'd be recursively inlining the same function,
         // which would provide the same callsites, which would cause us to
         // infinitely inline.
         InlineHistoryID = CallSites[CSi].second;
         if (InlineHistoryID != -1 &&
-            InlineHistoryIncludes(Callee, InlineHistoryID, InlineHistory))
+            InlineHistoryIncludes(Callee, InlineHistoryID, InlineHistory)) {
+          setInlineRemark(CS, "recursive");
           continue;
         }
+      }
 
       // FIXME for new PM: because of the old PM we currently generate ORE and
       // in turn BFI on demand.  With the new PM, the ORE dependency should
       // just become a regular analysis dependency.
       OptimizationRemarkEmitter ORE(Caller);
 
       Optional<InlineCost> OIC = shouldInline(CS, GetInlineCost, ORE);
       // If the policy determines that we should inline this function,
       // delete the call instead.
-      if (!OIC || !*OIC) {
+      if (!OIC.hasValue()) {
+        setInlineRemark(CS, "deferred");
+        continue;
+      }
+
+      if (!OIC.getValue()) {
+        // shouldInline() call returned a negative inline cost that explains
+        // why this callsite should not be inlined.
+        setInlineRemark(CS, inlineCostStr(*OIC));
         continue;
       }
 
       // If this call site is dead and it is to a readonly function, we should
       // just delete the call instead of trying to inline it, regardless of
       // size.  This happens because IPSCCP propagates the result out of the
       // call and then we're left with the dead call.
       if (IsTriviallyDead) {
         LLVM_DEBUG(dbgs() << "    -> Deleting dead call: " << *Instr << "\n");
         // Update the call graph by deleting the edge from Callee to Caller.
+        setInlineRemark(CS, "trivially dead");
         CG[Caller]->removeCallEdgeFor(CS);
         Instr->eraseFromParent();
         ++NumCallsDeleted;
       } else {
         // Get DebugLoc to report. CS will be invalid after Inliner.
         DebugLoc DLoc = CS->getDebugLoc();
         BasicBlock *Block = CS.getParent();
 
         // Attempt to inline the function.
         using namespace ore;
 
         InlineResult IR = InlineCallIfPossible(
             CS, InlineInfo, InlinedArrayAllocas, InlineHistoryID,
             InsertLifetime, AARGetter, ImportedFunctionsStats);
         if (!IR) {
+          setInlineRemark(CS, std::string(IR) + "; " + inlineCostStr(*OIC));
           ORE.emit([&]() {
             return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc,
                                             Block)
                    << NV("Callee", Callee) << " will not be inlined into "
                    << NV("Caller", Caller) << ": " << NV("Reason", IR.message);
           });
           continue;
         }
for (auto &N : InitialC) {
     // Instead we should do an actual RPO walk of the function body.
     for (Instruction &I : instructions(N.getFunction()))
       if (auto CS = CallSite(&I))
         if (Function *Callee = CS.getCalledFunction()) {
           if (!Callee->isDeclaration())
             Calls.push_back({CS, -1});
           else if (!isa<IntrinsicInst>(I)) {
             using namespace ore;
+            setInlineRemark(CS, "unavailable definition");
             ORE.emit([&]() {
               return OptimizationRemarkMissed(DEBUG_TYPE, "NoDefinition", &I)
                      << NV("Callee", Callee) << " will not be inlined into "
                      << NV("Caller", CS.getCaller())
                      << " because its definition is unavailable"
                      << setIsVerbose();
             });
           }
for (int i = 0; i < (int)Calls.size(); ++i) {
     // We expect the calls to typically be batched with sequences of calls that
     // have the same caller, so we first set up some shared infrastructure for
     // this caller. We also do any pruning we can at this layer on the caller
     // alone.
     Function &F = *Calls[i].first.getCaller();
     LazyCallGraph::Node &N = *CG.lookup(F);
     if (CG.lookupSCC(N) != C)
       continue;
-    if (F.hasFnAttribute(Attribute::OptimizeNone))
+    if (F.hasFnAttribute(Attribute::OptimizeNone)) {
+      setInlineRemark(Calls[i].first, "optnone attribute");
       continue;
+    }
 
     LLVM_DEBUG(dbgs() << "Inlining calls in: " << F.getName() << "\n");
 
     // Get a FunctionAnalysisManager via a proxy for this particular node. We
     // do this each time we visit a node as the SCC may have changed and as
     // we're going to mutate this particular function we want to make sure the
     // proxy is in place to forward any invalidation events. We can use the
     // manager we get here for looking up results for functions other than this
for (int i = 0; i < (int)Calls.size(); ++i) {
     bool DidInline = false;
     for (; i < (int)Calls.size() && Calls[i].first.getCaller() == &F; ++i) {
       int InlineHistoryID;
       CallSite CS;
       std::tie(CS, InlineHistoryID) = Calls[i];
       Function &Callee = *CS.getCalledFunction();
 
       if (InlineHistoryID != -1 &&
-          InlineHistoryIncludes(&Callee, InlineHistoryID, InlineHistory))
+          InlineHistoryIncludes(&Callee, InlineHistoryID, InlineHistory)) {
+        setInlineRemark(CS, "recursive");
         continue;
+      }
 
       // Check if this inlining may repeat breaking an SCC apart that has
       // already been split once before. In that case, inlining here may
       // trigger infinite inlining, much like is prevented within the inliner
       // itself by the InlineHistory above, but spread across CGSCC iterations
       // and thus hidden from the full inline history.
       if (CG.lookupSCC(*CG.lookup(Callee)) == C &&
           UR.InlinedInternalEdges.count({&N, C})) {
         LLVM_DEBUG(dbgs() << "Skipping inlining internal SCC edge from a node "
                              "previously split out of this SCC by inlining: "
                           << F.getName() << " -> " << Callee.getName() << "\n");
+        setInlineRemark(CS, "recursive SCC split");
         continue;
       }
 
       Optional<InlineCost> OIC = shouldInline(CS, GetInlineCost, ORE);
       // Check whether we want to inline this callsite.
-      if (!OIC || !*OIC)
+      if (!OIC.hasValue()) {
+        setInlineRemark(CS, "deferred");
         continue;
+      }
+
+      if (!OIC.getValue()) {
+        // shouldInline() call returned a negative inline cost that explains
+        // why this callsite should not be inlined.
+        setInlineRemark(CS, inlineCostStr(*OIC));
+        continue;
+      }
 
       // Setup the data structure used to plumb customization into the
       // `InlineFunction` routine.
       InlineFunctionInfo IFI(
           /*cg=*/nullptr, &GetAssumptionCache, PSI,
           &FAM.getResult<BlockFrequencyAnalysis>(*(CS.getCaller())),
           &FAM.getResult<BlockFrequencyAnalysis>(Callee));
 
       // Get DebugLoc to report. CS will be invalid after Inliner.
       DebugLoc DLoc = CS->getDebugLoc();
       BasicBlock *Block = CS.getParent();
 
       using namespace ore;
 
       InlineResult IR = InlineFunction(CS, IFI);
       if (!IR) {
+        setInlineRemark(CS, std::string(IR) + "; " + inlineCostStr(*OIC));
         ORE.emit([&]() {
           return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc, Block)
                  << NV("Callee", &Callee) << " will not be inlined into "
                  << NV("Caller", &F) << ": " << NV("Reason", IR.message);
         });
         continue;
       }
       DidInline = true;

llvm/trunk/test/Transforms/Inline/inline-remark.ll

+; RUN: opt < %s -inline -inline-remark-attribute --inline-threshold=-2 -S | FileCheck %s
+
+; Test that the inliner adds inline remark attributes to non-inlined callsites.
+
+define void @foo() {
+  call void @bar(i1 true)
+  ret void
+}
+
+define void @bar(i1 %p) {
+  br i1 %p, label %bb1, label %bb2
+
+bb1:
+  call void @foo()
+  ret void
+
+bb2:
+  call void @bar(i1 true)
+  ret void
+}
+
+;; Test 1 - Add different inline remarks to similar callsites.
+define void @test1() {
+; CHECK-LABEL: @test1
+; CHECK-NEXT: call void @bar(i1 true) [[ATTR1:#[0-9]+]]
+; CHECK-NEXT: call void @bar(i1 false) [[ATTR2:#[0-9]+]]
+  call void @bar(i1 true)
+  call void @bar(i1 false)
+  ret void
+}
+
+define void @noop() {
+  ret void
+}
+
+;; Test 2 - Printed InlineResult messages are followed by InlineCost.
+define void @test2(i8*) {
+; CHECK-LABEL: @test2
+; CHECK-NEXT: call void @noop() [[ATTR3:#[0-9]+]] [ "CUSTOM_OPERAND_BUNDLE"() ]
+; CHECK-NEXT: ret void
+  call void @noop() ; extepected to be inlined
+  call void @noop() [ "CUSTOM_OPERAND_BUNDLE"() ] ; cannot be inlined because of unsupported operand bundle
+  ret void
+}
+
+; CHECK: attributes [[ATTR1]] = { "inline-remark"="(cost=-5, threshold=-6)" }
+; CHECK: attributes [[ATTR2]] = { "inline-remark"="(cost=never): recursive" }
+; CHECK: attributes [[ATTR3]] = { "inline-remark"="unsupported operand bundle; (cost={{.*}}, threshold={{.*}})" }
+; RUN: opt < %s -inline -inline-remark-attribute --inline-threshold=-2 -S | FileCheck %s
+
+; Test that the inliner adds inline remark attributes to non-inlined callsites.
+
+define void @foo() {
+  call void @bar(i1 true)
+  ret void
+}
+
+define void @bar(i1 %p) {
+  br i1 %p, label %bb1, label %bb2
+
+bb1:
+  call void @foo()
+  ret void
+
+bb2:
+  call void @bar(i1 true)
+  ret void
+}
+
+;; Test 1 - Add different inline remarks to similar callsites.
+define void @test1() {
+; CHECK-LABEL: @test1
+; CHECK-NEXT: call void @bar(i1 true) [[ATTR1:#[0-9]+]]
+; CHECK-NEXT: call void @bar(i1 false) [[ATTR2:#[0-9]+]]
+  call void @bar(i1 true)
+  call void @bar(i1 false)
+  ret void
+}
+
+define void @noop() {
+  ret void
+}
+
+;; Test 2 - Printed InlineResult messages are followed by InlineCost.
+define void @test2(i8*) {
+; CHECK-LABEL: @test2
+; CHECK-NEXT: call void @noop() [[ATTR3:#[0-9]+]] [ "CUSTOM_OPERAND_BUNDLE"() ]
+; CHECK-NEXT: ret void
+  call void @noop() ; extepected to be inlined
+  call void @noop() [ "CUSTOM_OPERAND_BUNDLE"() ] ; cannot be inlined because of unsupported operand bundle
+  ret void
+}
+
+; CHECK: attributes [[ATTR1]] = { "inline-remark"="(cost=-5, threshold=-6)" }
+; CHECK: attributes [[ATTR2]] = { "inline-remark"="(cost=never): recursive" }
+; CHECK: attributes [[ATTR3]] = { "inline-remark"="unsupported operand bundle; (cost={{.*}}, threshold={{.*}})" }