[ThinLTO] Import readonly vars with refs

eleviant · eleviant · commit dde589389fcb · 2019-11-07T15:13:35.000+03:00
Patch allows importing declarations of functions and variables, referenced by the initializer of some other readonly variable. Differential revision: https://reviews.llvm.org/D69561
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -941,6 +941,11 @@ class ModuleSummaryIndex {
   /// considered live.
   bool WithGlobalValueDeadStripping = false;
 
+  /// Indicates that summary-based attribute propagation has run and
+  /// GVarFlags::MaybeReadonly / GVarFlags::MaybeWriteonly are really
+  /// read/write only.
+  bool WithAttributePropagation = false;
+
   /// Indicates that summary-based synthetic entry count propagation has run
   bool HasSyntheticEntryCounts = false;
 
@@ -1065,6 +1070,18 @@ class ModuleSummaryIndex {
     WithGlobalValueDeadStripping = true;
   }
 
+  bool withAttributePropagation() const { return WithAttributePropagation; }
+  void setWithAttributePropagation() {
+    WithAttributePropagation = true;
+  }
+
+  bool isReadOnly(const GlobalVarSummary *GVS) const {
+    return WithAttributePropagation && GVS->maybeReadOnly();
+  }
+  bool isWriteOnly(const GlobalVarSummary *GVS) const {
+    return WithAttributePropagation && GVS->maybeWriteOnly();
+  }
+
   bool hasSyntheticEntryCounts() const { return HasSyntheticEntryCounts; }
   void setHasSyntheticEntryCounts() { HasSyntheticEntryCounts = true; }
 
@@ -1356,6 +1373,9 @@ class ModuleSummaryIndex {
 
   /// Analyze index and detect unmodified globals
   void propagateAttributes(const DenseSet<GlobalValue::GUID> &PreservedSymbols);
+
+  /// Checks if we can import global variable from another module.
+  bool canImportGlobalVar(GlobalValueSummary *S, bool AnalyzeRefs) const;
 };
 
 /// GraphTraits definition to build SCC for the index
@@ -1427,15 +1447,6 @@ struct GraphTraits<ModuleSummaryIndex *> : public GraphTraits<ValueInfo> {
     return ValueInfo(I->haveGVs(), &P);
   }
 };
-
-static inline bool canImportGlobalVar(GlobalValueSummary *S) {
-  assert(isa<GlobalVarSummary>(S->getBaseObject()));
-
-  // We don't import GV with references, because it can result
-  // in promotion of local variables in the source module.
-  return !GlobalValue::isInterposableLinkage(S->linkage()) &&
-         !S->notEligibleToImport() && S->refs().empty();
-}
 } // end namespace llvm
 
 #endif // LLVM_IR_MODULESUMMARYINDEX_H
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -5761,7 +5761,7 @@ Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
   }
   const uint64_t Version = Record[0];
   const bool IsOldProfileFormat = Version == 1;
-  if (Version < 1 || Version > 7)
+  if (Version < 1 || Version > 8)
     return error("Invalid summary version " + Twine(Version) +
                  ". Version should be in the range [1-7].");
   Record.clear();
@@ -5814,7 +5814,7 @@ Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
     case bitc::FS_FLAGS: {  // [flags]
       uint64_t Flags = Record[0];
       // Scan flags.
-      assert(Flags <= 0x1f && "Unexpected bits in flag");
+      assert(Flags <= 0x3f && "Unexpected bits in flag");
 
       // 1 bit: WithGlobalValueDeadStripping flag.
       // Set on combined index only.
@@ -5837,6 +5837,10 @@ Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
       // Set on combined index only.
       if (Flags & 0x10)
         TheIndex.setPartiallySplitLTOUnits();
+      // 1 bit: WithAttributePropagation flag.
+      // Set on combined index only.
+      if (Flags & 0x20)
+        TheIndex.setWithAttributePropagation();
       break;
     }
     case bitc::FS_VALUE_GUID: { // [valueid, refguid]
@@ -6542,7 +6546,7 @@ static Expected<bool> getEnableSplitLTOUnitFlag(BitstreamCursor &Stream,
     case bitc::FS_FLAGS: { // [flags]
       uint64_t Flags = Record[0];
       // Scan flags.
-      assert(Flags <= 0x1f && "Unexpected bits in flag");
+      assert(Flags <= 0x3f && "Unexpected bits in flag");
 
       return Flags & 0x8;
     }
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -3723,7 +3723,7 @@ void ModuleBitcodeWriterBase::writeModuleLevelReferences(
 // Current version for the summary.
 // This is bumped whenever we introduce changes in the way some record are
 // interpreted, like flags for instance.
-static const uint64_t INDEX_VERSION = 7;
+static const uint64_t INDEX_VERSION = 8;
 
 /// Emit the per-module summary section alongside the rest of
 /// the module's bitcode.
@@ -3899,6 +3899,8 @@ void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
     Flags |= 0x8;
   if (Index.partiallySplitLTOUnits())
     Flags |= 0x10;
+  if (Index.withAttributePropagation())
+    Flags |= 0x20;
   Stream.EmitRecord(bitc::FS_FLAGS, ArrayRef<uint64_t>{Flags});
 
   for (const auto &GVI : valueIds()) {
diff --git a/llvm/lib/IR/ModuleSummaryIndex.cpp b/llvm/lib/IR/ModuleSummaryIndex.cpp
@@ -172,8 +172,9 @@ void ModuleSummaryIndex::propagateAttributes(
       // assembly leading it to be in the @llvm.*used).
       if (auto *GVS = dyn_cast<GlobalVarSummary>(S->getBaseObject()))
         // Here we intentionally pass S.get() not GVS, because S could be
-        // an alias.
-        if (!canImportGlobalVar(S.get()) ||
+        // an alias. We don't analyze references here, because we have to
+        // know exactly if GV is readonly to do so.
+        if (!canImportGlobalVar(S.get(), /* AnalyzeRefs */ false) ||
             GUIDPreservedSymbols.count(P.first)) {
           GVS->setReadOnly(false);
           GVS->setWriteOnly(false);
@@ -193,6 +194,23 @@ void ModuleSummaryIndex::propagateAttributes(
           }
 }
 
+bool ModuleSummaryIndex::canImportGlobalVar(GlobalValueSummary *S,
+                                            bool AnalyzeRefs) const {
+  auto HasRefsPreventingImport = [this](const GlobalVarSummary *GVS) {
+    return !isReadOnly(GVS) && GVS->refs().size();
+  };
+  auto *GVS = cast<GlobalVarSummary>(S->getBaseObject());
+
+  // Global variable with non-trivial initializer can be imported
+  // if it's readonly. This gives us extra opportunities for constant
+  // folding and converting indirect calls to direct calls. We don't
+  // analyze GV references during attribute propagation, because we
+  // don't know yet if it is readonly or not.
+  return !GlobalValue::isInterposableLinkage(S->linkage()) &&
+         !S->notEligibleToImport() &&
+         (!AnalyzeRefs || !HasRefsPreventingImport(GVS));
+}
+
 // TODO: write a graphviz dumper for SCCs (see ModuleSummaryIndex::exportToDot)
 // then delete this function and update its tests
 LLVM_DUMP_METHOD
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -280,7 +280,8 @@ updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) {
 }
 
 static void computeImportForReferencedGlobals(
-    const FunctionSummary &Summary, const GVSummaryMapTy &DefinedGVSummaries,
+    const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
+    const GVSummaryMapTy &DefinedGVSummaries,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
   for (auto &VI : Summary.refs()) {
@@ -303,16 +304,24 @@ static void computeImportForReferencedGlobals(
              RefSummary->modulePath() != Summary.modulePath();
     };
 
+    auto MarkExported = [&](const ValueInfo &VI, const GlobalValueSummary *S) {
+      if (ExportLists)
+        (*ExportLists)[S->modulePath()].insert(VI.getGUID());
+    };
+
     for (auto &RefSummary : VI.getSummaryList())
       if (isa<GlobalVarSummary>(RefSummary.get()) &&
-          canImportGlobalVar(RefSummary.get()) &&
+          Index.canImportGlobalVar(RefSummary.get(), /* AnalyzeRefs */ true) &&
           !LocalNotInModule(RefSummary.get())) {
         auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
         // Only update stat if we haven't already imported this variable.
         if (ILI.second)
           NumImportedGlobalVarsThinLink++;
-        if (ExportLists)
-          (*ExportLists)[RefSummary->modulePath()].insert(VI.getGUID());
+        MarkExported(VI, RefSummary.get());
+        // Promote referenced functions and variables
+        for (const auto &VI : RefSummary->refs())
+          for (const auto &RefFn : VI.getSummaryList())
+            MarkExported(VI, RefFn.get());
         break;
       }
   }
@@ -351,8 +360,8 @@ static void computeImportForFunction(
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists,
     FunctionImporter::ImportThresholdsTy &ImportThresholds) {
-  computeImportForReferencedGlobals(Summary, DefinedGVSummaries, ImportList,
-                                    ExportLists);
+  computeImportForReferencedGlobals(Summary, Index, DefinedGVSummaries,
+                                    ImportList, ExportLists);
   static int ImportCount = 0;
   for (auto &Edge : Summary.calls()) {
     ValueInfo VI = Edge.first;
@@ -864,6 +873,7 @@ void llvm::computeDeadSymbolsWithConstProp(
           GVS->setWriteOnly(false);
         }
   }
+  Index.setWithAttributePropagation();
 }
 
 /// Compute the set of summaries needed for a ThinLTO backend compilation of
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -238,7 +238,7 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   // If global value dead stripping is not enabled in summary then
   // propagateConstants hasn't been run. We can't internalize GV
   // in such case.
-  if (!GV.isDeclaration() && VI && ImportIndex.withGlobalValueDeadStripping()) {
+  if (!GV.isDeclaration() && VI && ImportIndex.withAttributePropagation()) {
     if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
       // We can have more than one local with the same GUID, in the case of
       // same-named locals in different but same-named source files that were
@@ -252,7 +252,7 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
       auto* GVS = dyn_cast_or_null<GlobalVarSummary>(
           ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
       // At this stage "maybe" is "definitely"
-      if (GVS && (GVS->maybeReadOnly() || GVS->maybeWriteOnly()))
+      if (GVS && (ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS)))
         V->addAttribute("thinlto-internalize");
     }
   }
diff --git a/llvm/test/Bitcode/summary_version.ll b/llvm/test/Bitcode/summary_version.ll
@@ -2,7 +2,7 @@
 ; RUN: opt  -module-summary  %s -o - | llvm-bcanalyzer -dump | FileCheck %s
 
 ; CHECK: <GLOBALVAL_SUMMARY_BLOCK
-; CHECK: <VERSION op0=7/>
+; CHECK: <VERSION op0=8/>
 
 
 
diff --git a/llvm/test/Bitcode/thinlto-deadstrip-flag.ll b/llvm/test/Bitcode/thinlto-deadstrip-flag.ll
@@ -5,14 +5,14 @@
 ; RUN: llvm-lto2 run %t.o -o %t.out -thinlto-distributed-indexes \
 ; RUN:		-r %t.o,glob,plx
 ; RUN: llvm-bcanalyzer -dump %t.o.thinlto.bc | FileCheck %s --check-prefix=WITHDEAD
-; WITHDEAD: <FLAGS op0=1/>
+; WITHDEAD: <FLAGS op0=33/>
 
 ; Ensure dead stripping performed flag is not set on distributed index
 ; when option used to disable dead stripping computation.
 ; RUN: llvm-lto2 run %t.o -o %t.out -thinlto-distributed-indexes \
 ; RUN:		-r %t.o,glob,plx -compute-dead=false
 ; RUN: llvm-bcanalyzer -dump %t.o.thinlto.bc | FileCheck %s --check-prefix=NODEAD
-; NODEAD: <FLAGS op0=0/>
+; NODEAD: <FLAGS op0=32/>
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/Bitcode/thinlto-synthetic-count-flag.ll b/llvm/test/Bitcode/thinlto-synthetic-count-flag.ll
@@ -5,15 +5,15 @@
 ; RUN: llvm-lto2 run %t.o -o %t.out -thinlto-distributed-indexes \
 ; RUN:		-r %t.o,glob,plx -compute-dead=false
 ; RUN: llvm-bcanalyzer -dump %t.o.thinlto.bc | FileCheck %s --check-prefix=NOSYNTHETIC
-; NOSYNTHETIC: <FLAGS op0=0/>
+; NOSYNTHETIC: <FLAGS op0=32/>
 
 ; Ensure synthetic entry count flag is set on distributed index
 ; when option used to enable synthetic count propagation
 ; RUN: llvm-lto2 run %t.o -o %t.out -thinlto-distributed-indexes \
 ; RUN:		-r %t.o,glob,plx -thinlto-synthesize-entry-counts \
 ; RUN:          -compute-dead=false
 ; RUN: llvm-bcanalyzer -dump %t.o.thinlto.bc | FileCheck %s --check-prefix=HASSYNTHETIC
-; HASSYNTHETIC: <FLAGS op0=4/>
+; HASSYNTHETIC: <FLAGS op0=36/>
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/ThinLTO/X86/globals-import.ll b/llvm/test/ThinLTO/X86/globals-import.ll
@@ -15,7 +15,7 @@
 ; RUN: llvm-dis %t2.bc.thinlto.promoted.bc -o - | FileCheck --check-prefix=PROMOTE1 %s
 ; RUN: llvm-dis %t2b.bc.thinlto.promoted.bc -o - | FileCheck --check-prefix=PROMOTE2 %s
 
-; IMPORT: @baz.llvm.0 = available_externally hidden constant i32 10, align 4
+; IMPORT: @baz.llvm.0 = internal constant i32 10, align 4
 
 ; PROMOTE1: @baz.llvm.0 = hidden constant i32 10, align 4
 ; PROMOTE1: define weak_odr i32 @foo() {
diff --git a/llvm/test/ThinLTO/X86/local_name_conflict.ll b/llvm/test/ThinLTO/X86/local_name_conflict.ll
@@ -12,7 +12,7 @@
 ; that module (%t3.bc) to be imported. Check that the imported reference's
 ; promoted name matches the imported copy.
 ; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
-; IMPORT: @baz.llvm.[[HASH:[0-9]+]] = available_externally hidden constant i32 10, align 4
+; IMPORT: @baz.llvm.[[HASH:[0-9]+]] = internal constant i32 10, align 4
 ; IMPORT: call i32 @foo.llvm.[[HASH]]
 ; IMPORT: define available_externally hidden i32 @foo.llvm.[[HASH]]()
 
diff --git a/llvm/test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll b/llvm/test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll
@@ -35,7 +35,7 @@
 ; should not be set.
 ; RUN: llvm-bcanalyzer --dump %t1.o.thinlto.bc | FileCheck %s -check-prefixes=CHECK-BC1
 ; CHECK-BC1: <GLOBALVAL_SUMMARY_BLOCK
-; CHECK-BC1: <FLAGS op0=1/>
+; CHECK-BC1: <FLAGS op0=33/>
 ; CHECK-BC1: </GLOBALVAL_SUMMARY_BLOCK
 
 ; Nothing interesting in the corresponding object file, so
