ThinLTO : Import always_inline functions irrespective of the threshold

[Bharathi]

Summary

A user can force a function to be inlined by specifying the always_inline attribute. Currently, thinlto implementation is not aware of always_inline functions and does not guarantee import of such functions, which in turn can prevent inlining of such functions.

[tejohnson]

I just started to look at this (added a minor comment below) - but could you please re-upload this with full context so that it is easier to review?

llvm/include/llvm/IR/ModuleSummaryIndex.h

550

document

[hiraditya]

llvm/test/ThinLTO/X86/Inputs/funcimport_alwaysinline.ll

10

the definition of \#1 and \#0 seems to be missing?

[tejohnson]

llvm/test/ThinLTO/X86/Inputs/funcimport_alwaysinline.ll

10

Probably can just remove the attribute numbers here since they aren't needed.

[Bharathi]

Uploaded with full context and addressed review comments received so far.

[tejohnson]

Thanks for working on this! One suggestion for test cleanup below.

llvm/test/ThinLTO/X86/funcimport_alwaysinline.ll

30

I don't think you need all these calls to @bar (or any, or the def of @bar). I believe this was from another test that was testing the import-instr-limit. Since you have 0 you don't need anything here.

[Bharathi]

Clean up test case.

[tejohnson]

LGTM thanks!

llvm/include/llvm/IR/ModuleSummaryIndex.h

 
     // Indicate if the global value cannot be inlined.
     unsigned NoInline : 1;
+    unsigned AlwaysInline : 1;
   };
 
   /// Create an empty FunctionSummary (with specified call edges).

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

       F.hasFnAttribute(Attribute::NoRecurse), F.returnDoesNotAlias(),
       // FIXME: refactor this to use the same code that inliner is using.
       // Don't try to import functions with noinline attribute.
-      F.getAttributes().hasFnAttribute(Attribute::NoInline)};
+      F.getAttributes().hasFnAttribute(Attribute::NoInline),
+      F.hasFnAttribute(Attribute::AlwaysInline)};
   auto FuncSummary = std::make_unique<FunctionSummary>(
       Flags, NumInsts, FunFlags, /*EntryCount=*/0, std::move(Refs),
       CallGraphEdges.takeVector(), TypeTests.takeVector(),

                         F->hasFnAttribute(Attribute::ReadOnly),
                         F->hasFnAttribute(Attribute::NoRecurse),
                         F->returnDoesNotAlias(),
-                        /* NoInline = */ false},
+                        /* NoInline = */ false,
+                        F->hasFnAttribute(Attribute::AlwaysInline)},
                     /*EntryCount=*/0, ArrayRef<ValueInfo>{},
                     ArrayRef<FunctionSummary::EdgeTy>{},
                     ArrayRef<GlobalValue::GUID>{},

llvm/lib/AsmParser/LLLexer.cpp

   KEYWORD(noRecurse);
   KEYWORD(returnDoesNotAlias);
   KEYWORD(noInline);
+  KEYWORD(alwaysInline);
   KEYWORD(calls);
   KEYWORD(callee);
   KEYWORD(hotness);

llvm/lib/AsmParser/LLParser.cpp

 ///        [',' 'readOnly' ':' Flag]? [',' 'noRecurse' ':' Flag]?
 ///        [',' 'returnDoesNotAlias' ':' Flag]? ')'
 ///        [',' 'noInline' ':' Flag]? ')'
+///        [',' 'alwaysInline' ':' Flag]? ')'
+
 bool LLParser::ParseOptionalFFlags(FunctionSummary::FFlags &FFlags) {
   assert(Lex.getKind() == lltok::kw_funcFlags);
   Lex.Lex();

         return true;
       FFlags.NoInline = Val;
       break;
+    case lltok::kw_alwaysInline:
+      Lex.Lex();
+      if (ParseToken(lltok::colon, "expected ':'") || ParseFlag(Val))
+        return true;
+      FFlags.AlwaysInline = Val;
+      break;
     default:
       return Error(Lex.getLoc(), "expected function flag type");
     }

llvm/lib/AsmParser/LLToken.h

   kw_noRecurse,
   kw_returnDoesNotAlias,
   kw_noInline,
+  kw_alwaysInline,
   kw_calls,
   kw_callee,
   kw_hotness,

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

   Flags.NoRecurse = (RawFlags >> 2) & 0x1;
   Flags.ReturnDoesNotAlias = (RawFlags >> 3) & 0x1;
   Flags.NoInline = (RawFlags >> 4) & 0x1;
+  Flags.AlwaysInline = (RawFlags >> 5) & 0x1;
   return Flags;
 }
 

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

   RawFlags |= (Flags.NoRecurse << 2);
   RawFlags |= (Flags.ReturnDoesNotAlias << 3);
   RawFlags |= (Flags.NoInline << 4);
+  RawFlags |= (Flags.AlwaysInline << 5);
   return RawFlags;
 }
 

llvm/lib/IR/AsmWriter.cpp

     Out << ", noRecurse: " << FFlags.NoRecurse;
     Out << ", returnDoesNotAlias: " << FFlags.ReturnDoesNotAlias;
     Out << ", noInline: " << FFlags.NoInline;
+    Out << ", alwaysInline: " << FFlags.AlwaysInline;
     Out << ")";
   }
   if (!FS->calls().empty()) {

llvm/lib/IR/ModuleSummaryIndex.cpp

   auto FlagValue = [](unsigned V) { return V ? '1' : '0'; };
   char FlagRep[] = {FlagValue(F.ReadNone),     FlagValue(F.ReadOnly),
                     FlagValue(F.NoRecurse),    FlagValue(F.ReturnDoesNotAlias),
-                    FlagValue(F.NoInline), 0};
+                    FlagValue(F.NoInline), FlagValue(F.AlwaysInline), 0};
 
   return FlagRep;
 }

llvm/lib/Transforms/IPO/FunctionImport.cpp

           return false;
         }
 
-        if (Summary->instCount() > Threshold) {
+        if ((Summary->instCount() > Threshold) &&
+            !Summary->fflags().AlwaysInline) {
           Reason = FunctionImporter::ImportFailureReason::TooLarge;
           return false;
         }

       CalleeSummary = CalleeSummary->getBaseObject();
       ResolvedCalleeSummary = cast<FunctionSummary>(CalleeSummary);
 
-      assert(ResolvedCalleeSummary->instCount() <= NewThreshold &&
+      assert((ResolvedCalleeSummary->fflags().AlwaysInline ||
+       (ResolvedCalleeSummary->instCount() <= NewThreshold)) &&
              "selectCallee() didn't honor the threshold");
 
       auto ExportModulePath = ResolvedCalleeSummary->modulePath();

llvm/test/Assembler/thinlto-summary.ll

 ; Functions with various flag combinations (notEligibleToImport, Live,
 ; combinations of optional function flags).
 ^15 = gv: (guid: 14, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 1, live: 1, dsoLocal: 0), insts: 1, funcFlags: (noInline: 1))))
-^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 1, funcFlags: (readNone: 1, noRecurse: 1))))
+^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 1, funcFlags: (readNone: 1, noRecurse: 1, alwaysInline: 1))))
 ; This one also tests backwards reference in calls.
 ^17 = gv: (guid: 16, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0), insts: 1, funcFlags: (readOnly: 1, returnDoesNotAlias: 1), calls: ((callee: ^15)))))
 

 ; CHECK: ^12 = gv: (guid: 11, summaries: (variable: (module: ^0, flags: (linkage: appending, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0), refs: (^4))))
 ; CHECK: ^13 = gv: (guid: 12, summaries: (variable: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), varFlags: (readonly: 1, writeonly: 0))))
 ; CHECK: ^14 = gv: (guid: 13, summaries: (variable: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0))))
-; CHECK: ^15 = gv: (guid: 14, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 1, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 0, returnDoesNotAlias: 0, noInline: 1))))
-; CHECK: ^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0))))
-; CHECK: ^17 = gv: (guid: 16, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 1, noRecurse: 0, returnDoesNotAlias: 1, noInline: 0), calls: ((callee: ^15)))))
+; CHECK: ^15 = gv: (guid: 14, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 1, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 0, returnDoesNotAlias: 0, noInline: 1, alwaysInline: 0))))
+; CHECK: ^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 1))))
+; CHECK: ^17 = gv: (guid: 16, summaries: (function: (module: ^1, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 1, noRecurse: 0, returnDoesNotAlias: 1, noInline: 0, alwaysInline: 0), calls: ((callee: ^15)))))
 ; CHECK: ^18 = gv: (guid: 17, summaries: (alias: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), aliasee: ^14)))
 ; CHECK: ^19 = gv: (guid: 18, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 4, typeIdInfo: (typeTests: (^24, ^26)))))
 ; CHECK: ^20 = gv: (guid: 19, summaries: (function: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 8, typeIdInfo: (typeTestAssumeVCalls: (vFuncId: (^27, offset: 16))))))

llvm/test/ThinLTO/X86/Inputs/funcimport_alwaysinline.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main() #0 {
+entry:
+  call void (...) @foo()
+  ret i32 0
+}
+
+declare void @foo(...) #1

llvm/test/ThinLTO/X86/dot-dumper.ll

 ; PERMODULE-NEXT:    label = "";
 ; PERMODULE-NEXT:    node [style=filled,fillcolor=lightblue];
 ; PERMODULE-NEXT:    M0_[[MAIN_ALIAS:[0-9]+]] [style="dotted,filled",shape="box",label="main_alias",fillcolor="red"]; // alias, dead
-; PERMODULE-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 4, ffl: 00000)}",fillcolor="red"]; // function, dead
+; PERMODULE-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 4, ffl: 000000)}",fillcolor="red"]; // function, dead
 ; PERMODULE-NEXT:    // Edges:
 ; PERMODULE-NEXT:    M0_[[MAIN_ALIAS]] -> M0_[[MAIN]] [style=dotted]; // alias
 ; PERMODULE-NEXT:  }

 ; COMBINED-NEXT:    label = "dot-dumper{{.*}}1.bc";
 ; COMBINED-NEXT:    node [style=filled,fillcolor=lightblue];
 ; COMBINED-NEXT:    M0_[[MAIN_ALIAS:[0-9]+]] [style="dotted,filled",shape="box",label="main_alias",fillcolor="red"]; // alias, dead
-; COMBINED-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 4, ffl: 00000)}"]; // function
+; COMBINED-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 4, ffl: 000000)}"]; // function
 ; COMBINED-NEXT:    // Edges:
 ; COMBINED-NEXT:    M0_[[MAIN_ALIAS]] -> M0_[[MAIN]] [style=dotted]; // alias
 ; COMBINED-NEXT:  }

 ; COMBINED-NEXT:    color = lightgrey;
 ; COMBINED-NEXT:    label = "dot-dumper{{.*}}2.bc";
 ; COMBINED-NEXT:    node [style=filled,fillcolor=lightblue];
-; COMBINED-NEXT:    M1_[[FOO:[0-9]+]] [shape="record",label="foo|extern (inst: 4, ffl: 00001)}"]; // function
+; COMBINED-NEXT:    M1_[[FOO:[0-9]+]] [shape="record",label="foo|extern (inst: 4, ffl: 000010)}"]; // function
 ; COMBINED-NEXT:    M1_[[A:[0-9]+]] [shape="Mrecord",label="A|extern}"]; // variable, immutable
 ; COMBINED-NEXT:    M1_[[B:[0-9]+]] [shape="Mrecord",label="B|extern}"]; // variable, immutable
-; COMBINED-NEXT:    M1_{{[0-9]+}} [shape="record",label="bar|extern (inst: 1, ffl: 00000)}",fillcolor="red"]; // function, dead
+; COMBINED-NEXT:    M1_{{[0-9]+}} [shape="record",label="bar|extern (inst: 1, ffl: 000000)}",fillcolor="red"]; // function, dead
 ; COMBINED-NEXT:    // Edges:
 ; COMBINED-NEXT:    M1_[[FOO]] -> M1_[[B]] [style=dashed,color=forestgreen]; // const-ref
 ; COMBINED-NEXT:    M1_[[FOO]] -> M1_[[A]] [style=dashed,color=forestgreen]; // const-ref

llvm/test/ThinLTO/X86/dot-dumper2.ll

 ; COMBINED-NEXT:    color = lightgrey;
 ; COMBINED-NEXT:    label =
 ; COMBINED-NEXT:    node [style=filled,fillcolor=lightblue];
-; COMBINED-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 2, ffl: 00000)}"]; // function
+; COMBINED-NEXT:    M0_[[MAIN:[0-9]+]] [shape="record",label="main|extern (inst: 2, ffl: 000000)}"]; // function
 ; COMBINED-NEXT:    // Edges:
 ; COMBINED-NEXT:  }
 ; COMBINED-NEXT:  // Module:

llvm/test/ThinLTO/X86/funcimport_alwaysinline.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/funcimport_alwaysinline.ll -o %t2.bc
+
+; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,foo,plx \
+; RUN:     -r=%t1.bc,bar,l \
+; RUN:     -r=%t2.bc,main,plx \
+; RUN:     -r=%t2.bc,foo,l \
+; RUN:     -import-instr-limit=0
+; RUN: llvm-dis %t.o.2.3.import.bc -o - | FileCheck %s
+
+; foo() being always_inline should be imported irrespective of the
+; instruction limit
+; CHECK: define available_externally dso_local void @foo()
+; CHECK: declare dso_local void @bar()
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define void @bar() #0 {
+entry:
+  ret void
+}
+
+; Function Attrs: alwaysinline nounwind uwtable
+define void @foo() #1 {
+entry:
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  call void @bar()
+  ret void
+}
+
+attributes #0 = { nounwind uwtable }
+attributes #1 = { alwaysinline nounwind uwtable }