[ThinLTO] Make inline assembly handling more efficient in summary

Summary:
The change in r285513 to prevent exporting of locals used in
inline asm added all locals in the llvm.used set to the reference
set of functions containing inline asm. Since these locals were marked
NoRename, this automatically prevented importing of the function.

Unfortunately, this caused an explosion in the summary reference lists
in some cases. In my particular example, it happened for a large protocol
buffer generated C++ file, where many of the generated functions
contained an inline asm call. It was exacerbated when doing a ThinLTO
PGO instrumentation build, where the PGO instrumentation included
thousands of private __profd_* values that were added to llvm.used.

We really only need to include a single llvm.used local (NoRename) value
in the reference list of a function containing inline asm to block it
being imported. However, it seems cleaner to add a flag to the summary
that explicitly describes this situation, which is what this patch does.

Reviewers: mehdi_amini

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D26402

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@286840 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/IR/ModuleSummaryIndex.h b/include/llvm/IR/ModuleSummaryIndex.h
index b3775904e75746e4bf94522afc04023095704300..97875b6de58188c9450899564c41d21a1f07fcb7 100644 (file)
--- a/include/llvm/IR/ModuleSummaryIndex.h
+++ b/include/llvm/IR/ModuleSummaryIndex.h
@@ -101,17 +101,29 @@ public:
     /// possibly referenced from inline assembly, etc).
     unsigned NoRename : 1;
 
+    /// Indicate if a function contains inline assembly (which is opaque),
+    /// that may reference a local value. This is used to prevent importing
+    /// of this function, since we can't promote and rename the uses of the
+    /// local in the inline assembly. Use a flag rather than bloating the
+    /// summary with references to every possible local value in the
+    /// llvm.used set.
+    unsigned HasInlineAsmMaybeReferencingInternal : 1;
+
     /// Indicate if the function is not viable to inline.
     unsigned IsNotViableToInline : 1;
 
     /// Convenience Constructors
     explicit GVFlags(GlobalValue::LinkageTypes Linkage, bool NoRename,
+                     bool HasInlineAsmMaybeReferencingInternal,
                      bool IsNotViableToInline)
         : Linkage(Linkage), NoRename(NoRename),
+          HasInlineAsmMaybeReferencingInternal(
+              HasInlineAsmMaybeReferencingInternal),
           IsNotViableToInline(IsNotViableToInline) {}
 
     GVFlags(const GlobalValue &GV)
-        : Linkage(GV.getLinkage()), NoRename(GV.hasSection()) {
+        : Linkage(GV.getLinkage()), NoRename(GV.hasSection()),
+          HasInlineAsmMaybeReferencingInternal(false) {
       IsNotViableToInline = false;
       if (const auto *F = dyn_cast<Function>(&GV))
         // Inliner doesn't handle variadic functions.
@@ -198,6 +210,18 @@ public:
   /// possibly referenced from inline assembly, etc).
   void setNoRename() { Flags.NoRename = true; }
 
+  /// Return true if this global value possibly references another value
+  /// that can't be renamed.
+  bool hasInlineAsmMaybeReferencingInternal() const {
+    return Flags.HasInlineAsmMaybeReferencingInternal;
+  }
+
+  /// Flag that this global value possibly references another value that
+  /// can't be renamed.
+  void setHasInlineAsmMaybeReferencingInternal() {
+    Flags.HasInlineAsmMaybeReferencingInternal = true;
+  }
+
   /// Record a reference from this global value to the global value identified
   /// by \p RefGUID.
   void addRefEdge(GlobalValue::GUID RefGUID) { RefEdgeList.push_back(RefGUID); }

diff --git a/lib/Analysis/ModuleSummaryAnalysis.cpp b/lib/Analysis/ModuleSummaryAnalysis.cpp
index a3f43071b44c4442e88607e716880d178fa432a1..0c07e1d878f68150c5dcdbe5887259ee7f00882c 100644 (file)
--- a/lib/Analysis/ModuleSummaryAnalysis.cpp
+++ b/lib/Analysis/ModuleSummaryAnalysis.cpp
@@ -78,7 +78,7 @@ static CalleeInfo::HotnessType getHotness(uint64_t ProfileCount,
 static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
                                    const Function &F, BlockFrequencyInfo *BFI,
                                    ProfileSummaryInfo *PSI,
-                                   SmallPtrSetImpl<GlobalValue *> &LocalsUsed) {
+                                   bool HasLocalsInUsed) {
   // Summary not currently supported for anonymous functions, they should
   // have been named.
   assert(F.hasName());
@@ -90,8 +90,8 @@ static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
   DenseMap<GlobalValue::GUID, CalleeInfo> IndirectCallEdges;
   DenseSet<const Value *> RefEdges;
   ICallPromotionAnalysis ICallAnalysis;
-  bool HasLocalsInUsed = !LocalsUsed.empty();
 
+  bool HasInlineAsmMaybeReferencingInternal = false;
   SmallPtrSet<const User *, 8> Visited;
   for (const BasicBlock &BB : F)
     for (const Instruction &I : BB) {
@@ -105,11 +105,12 @@ static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
 
       const auto *CI = dyn_cast<CallInst>(&I);
       // Since we don't know exactly which local values are referenced in inline
-      // assembly, conservatively reference all of them from this function, to
-      // ensure we don't export a reference (which would require renaming and
-      // promotion).
+      // assembly, conservatively mark the function as possibly referencing
+      // a local value from inline assembly to ensure we don't export a
+      // reference (which would require renaming and promotion of the
+      // referenced value).
       if (HasLocalsInUsed && CI && CI->isInlineAsm())
-        RefEdges.insert(LocalsUsed.begin(), LocalsUsed.end());
+        HasInlineAsmMaybeReferencingInternal = true;
 
       auto *CalledValue = CS.getCalledValue();
       auto *CalledFunction = CS.getCalledFunction();
@@ -162,6 +163,8 @@ static void computeFunctionSummary(ModuleSummaryIndex &Index, const Module &M,
   FuncSummary->addCallGraphEdges(CallGraphEdges);
   FuncSummary->addCallGraphEdges(IndirectCallEdges);
   FuncSummary->addRefEdges(RefEdges);
+  if (HasInlineAsmMaybeReferencingInternal)
+    FuncSummary->setHasInlineAsmMaybeReferencingInternal();
   Index.addGlobalValueSummary(F.getName(), std::move(FuncSummary));
 }
 
@@ -232,7 +235,7 @@ ModuleSummaryIndex llvm::buildModuleSummaryIndex(
       BFI = BFIPtr.get();
     }
 
-    computeFunctionSummary(Index, M, F, BFI, PSI, LocalsUsed);
+    computeFunctionSummary(Index, M, F, BFI, PSI, !LocalsUsed.empty());
   }
 
   // Compute summaries for all variables defined in module, and save in the

diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp
index bf1f6a56fa10470480762bab530c0683a7728fdd..94a3bcf85540f7e3601279e732d17b890f9934aa 100644 (file)
--- a/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -946,7 +946,10 @@ static GlobalValueSummary::GVFlags getDecodedGVSummaryFlags(uint64_t RawFlags,
   RawFlags = RawFlags >> 4;
   bool NoRename = RawFlags & 0x1;
   bool IsNotViableToInline = RawFlags & 0x2;
-  return GlobalValueSummary::GVFlags(Linkage, NoRename, IsNotViableToInline);
+  bool HasInlineAsmMaybeReferencingInternal = RawFlags & 0x4;
+  return GlobalValueSummary::GVFlags(Linkage, NoRename,
+                                     HasInlineAsmMaybeReferencingInternal,
+                                     IsNotViableToInline);
 }
 
 static GlobalValue::VisibilityTypes getDecodedVisibility(unsigned Val) {

diff --git a/lib/Bitcode/Writer/BitcodeWriter.cpp b/lib/Bitcode/Writer/BitcodeWriter.cpp
index 288c7c6fac29c74b2ad5aacd2dd49c2505aa3f30..80db5b2e061da2d89bc1bca82808c4e82d29c34c 100644 (file)
--- a/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -992,6 +992,7 @@ static uint64_t getEncodedGVSummaryFlags(GlobalValueSummary::GVFlags Flags) {
 
   RawFlags |= Flags.NoRename; // bool
   RawFlags |= (Flags.IsNotViableToInline << 1);
+  RawFlags |= (Flags.HasInlineAsmMaybeReferencingInternal << 2);
   // Linkage don't need to be remapped at that time for the summary. Any future
   // change to the getEncodedLinkage() function will need to be taken into
   // account here as well.

diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
index 3bebc609bea657113e08940e44828b4875bbd04e..68626c020380ff21be86e5529ee39745d8a4d770 100644 (file)
--- a/lib/Transforms/IPO/FunctionImport.cpp
+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -153,7 +153,11 @@ static bool eligibleForImport(const ModuleSummaryIndex &Index,
 
   // Check references (and potential calls) in the same module. If the current
   // value references a global that can't be externally referenced it is not
-  // eligible for import.
+  // eligible for import. First check the flag set when we have possible
+  // opaque references (e.g. inline asm calls), then check the call and
+  // reference sets.
+  if (Summary.hasInlineAsmMaybeReferencingInternal())
+    return false;
   bool AllRefsCanBeExternallyReferenced =
       llvm::all_of(Summary.refs(), [&](const ValueInfo &VI) {
         return canBeExternallyReferenced(Index, VI.getGUID());