[ThinLTO] Import readonly vars with refs

[evgeny777]

Summary

This patch allows importing readonly variables which have initializer referencing other functions or variables. Referenced functions (or vars) are promoted but not imported.

This allows two things to be done:

• Replacing indirect calls with direct calls to functions referenced in initializer
• Additional DCE with -Wl,-mllvm,-function-sections -Wl,-mllvm,-data-sections, -Wl,--gc-sections linker flags

[steven_wu]

I can see why this is a good idea, but do you have numbers for this optimization? How much code can be inlined? Benchmark for speedup? code size? compile time impact?

lib/Transforms/IPO/FunctionImport.cpp

282 ↗

(On Diff #226884)

I will just make function to be type bool hasRefsPreventingImport(const GlobalVarSummary*);. The callsite already has isa check so it will be much better to type casting there.

[evgeny777]

I can see why this is a good idea, but do you have numbers for this optimization? How much code can be inlined?

This patch doesn't directly affect inlining, though it can reduce number of IR instructions and make inline cost of a function lower.
LLVM test suite shows noticeable code size reduction of Bullet test case (~1%), because deallocation function pointer sFreeFunc is imported with initializer
and load instruction gets eliminated in few destructors (indirect call is converted to direct call). On the whole the change introduced by this patch is quite small
and I wouldn't expect much from it. It would probably work better for C programs, where function pointers in structures is common OO pattern.

Also it may become better with better readonly'ness detection (I have plans for it as well).

Benchmark for speedup?

I've observed slightly better execution time of MultiSource test suite over several runs, but this is most likely some sort of caching issue.

code size?

See above. There are minor size improvements for other MultiSource benchmarks, but too small to be considered.

compile time impact?

Not observed. Typical program contains few global variables (compared to number of functions), and very few of them are readonly. Among those few have non-trivial initializer.

[evgeny777]

Addressed. See comment above for performance insights

[steven_wu]

LGTM.

[tejohnson]

Thanks for working on this! I had been thinking about adding this recently but didn't have a chance yet. I have a few suggestions below. One thing that should be considered for a follow-up enhancement is to consider the refs themselves for importing (similar to the way we follow chains of calls when doing function importing).

lib/Transforms/IPO/FunctionImport.cpp

283 ↗

(On Diff #227099)

The name of the maybeReadOnly is a little confusing here, although I know we expect that analysis to have finalized this flag by now. Can you add a flag to the summary index to indicate that the attribute propagation has completed (similar to the WithGlobalValueDeadStripping flag that indicates liveness analysis is complete), and then add a helper on the summary to return the maybeReadOnly value that asserts the flag is set, and change the uses sites to use that instead? E.g. something like a readOnly() method, and probably writeOnly() one for completeness.

318 ↗

(On Diff #227099)

I think it would be better to call this from within canImportGlobalVar to keep the correctness checks centralized.

[evgeny777]

Addressed.

Adding new flag to summary index allowed better handling cases when actual dead symbol computation was skipped for some reason. See modifications in processGlobalForThinLTO and updated test cases.

One thing that should be considered for a follow-up enhancement is to consider the refs themselves for importing (similar to the way we follow chains of calls when doing function importing).

This has to be done with caution, because importing everything actually makes things worse due to promotion in source modules.
One thing which is worth doing is calculating references from initializer to constant object, e.g:

static const struct A { ... } objA = { ... };
struct B { A *pA; } objB = { &objA };

and import such references.

Another thing which seems also worth doing is computing functions which doesn't modify their pointer arguments. Calling such functions with
global object pointer doesn't violate readonly property. Also this could be a base for some other optimizations (like argument promotion).

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

5853 ↗

(On Diff #227288)

I don't follow the second check, since the WithGlobalValueDeadStripping flag was added to the index before attribute propagation was supported.

lib/IR/ModuleSummaryIndex.cpp

201 ↗

(On Diff #227288)

I had forgotten that this was called during attribute propagation. Won't guarding this check with WithAttributePropagation do the wrong thing during importing if we somehow skipped attribute propagation (doesn't look like that can happen today, but let's say we add a debugging option in the future to allow that to be skipped). It would be safer to have this method take a flag that indicates whether we are doing this during attribute propagation, and if not then return true if !WithAttributePropagation.

205 ↗

(On Diff #227288)

Update comment

lib/Transforms/IPO/FunctionImport.cpp

865 ↗

(On Diff #227288)

Where/how does this happen? There are two callsites, with one using an empty set of preserved symbols (old LTO API), but they are for the old and new LTO APIs, and we would never be calling both.

It does look like this can potentially be called multiple times when testing through llvm-lto (because the testing facilities there like crossModuleImport(), internalize(), etc) will be called once per input file, but I don't see it otherwise being called twice. If this is just because of llvm-lto testing, can you update the comment to indicate that this is called multiple times when testing via the old LTO API (I don't see it happening via the new LTO API, but maybe I am missing something).

test/ThinLTO/X86/local_name_conflict.ll

15 ↗

(On Diff #227288)

I assume this is what you were referring to in your comments about the change to processGlobalForThinLTO for cases when actual dead symbol computation was skipped for some reason, but I don't see why we would be skipping the dead symbol computation here - llvm-lto's import action invokes the old LTO crossModuleImport() which ultimately calls computeDeadSymbolsWithConstProp, and I'm not sure why that would do the attribute propagation but not the dead symbol computation.

[evgeny777]

lib/IR/ModuleSummaryIndex.cpp

201 ↗

(On Diff #227288)

Makes sense.

lib/Transforms/IPO/FunctionImport.cpp

865 ↗

(On Diff #227288)

computeDeadSymbols is currently called multiple times from llvm-lto with empty GUIDPreservedSymbols (see emit_imports.ll) and can *potemtially* be called with non-empty GUIDPreservedSymbols second time (although *currently* this doesn't happen). However if we add some boolean flag, like IsCalledFromPropagation to canImportGlobalVar then this doesn't really matter much.

test/ThinLTO/X86/local_name_conflict.ll

15 ↗

(On Diff #227288)

This is because on some occasions WithGlobalValueDeadStripping can be false and WithAttibutePropagation can be true. One of such cases is when GUIDPreservedSymbols is empty. In such case we consider all symbols live. Having all symbols live doesn't however mean that some of the can't be readonly. That's why we see some additional internalizations happening.

[evgeny777]

lib/Bitcode/Reader/BitcodeReader.cpp

5853 ↗

(On Diff #227288)

We have constant propagation in index since version 5 and if value of true of WithGlobalValueDeadStripping flag means constant propagation has run. I tend to remove this check for the sake of simplicity though.

[evgeny777]

Addressed and rebased

[tejohnson]

LGTM with some minor comment changes suggested. Thanks!

llvm/lib/IR/ModuleSummaryIndex.cpp

175–177

document constant param

llvm/lib/Transforms/IPO/FunctionImport.cpp

314

document const param

864

This comment is stale and can presumably be removed now, as the corresponding setWithAttributePropagation(false) has been removed, right?

llvm/include/llvm/IR/ModuleSummaryIndex.h

private:
   /// GUIDs, it will be mapped to 0.
   std::map<GlobalValue::GUID, GlobalValue::GUID> OidGuidMap;
 
   /// Indicates that summary-based GlobalValue GC has run, and values with
   /// GVFlags::Live==false are really dead. Otherwise, all values must be
   /// 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;
 
   /// Indicates that distributed backend should skip compilation of the
   /// module. Flag is suppose to be set by distributed ThinLTO indexing
   /// when it detected that the module is not needed during the final
   /// linking. As result distributed backend should just output a minimal
   /// valid object file.
public:
 
   bool withGlobalValueDeadStripping() const {
     return WithGlobalValueDeadStripping;
   }
   void setWithGlobalValueDeadStripping() {
     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; }
 
   bool skipModuleByDistributedBackend() const {
     return SkipModuleByDistributedBackend;
   }
   void setSkipModuleByDistributedBackend() {
     SkipModuleByDistributedBackend = true;
public:
   /// Export summary to dot file for GraphViz.
   void exportToDot(raw_ostream& OS) const;
 
   /// Print out strongly connected components for debugging.
   void dumpSCCs(raw_ostream &OS);
 
   /// 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
 template <> struct GraphTraits<ValueInfo> {
   typedef ValueInfo NodeRef;
   using EdgeRef = FunctionSummary::EdgeTy &;
 
   static NodeRef valueInfoFromEdge(FunctionSummary::EdgeTy &P) {
std::unique_ptr<GlobalValueSummary> Root =
         std::make_unique<FunctionSummary>(I->calculateCallGraphRoot());
     GlobalValueSummaryInfo G(I->haveGVs());
     G.SummaryList.push_back(std::move(Root));
     static auto P =
         GlobalValueSummaryMapTy::value_type(GlobalValue::GUID(0), std::move(G));
     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

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

// Parse version
     Expected<unsigned> MaybeRecord = Stream.readRecord(Entry.ID, Record);
     if (!MaybeRecord)
       return MaybeRecord.takeError();
     if (MaybeRecord.get() != bitc::FS_VERSION)
       return error("Invalid Summary Block: version expected");
   }
   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();
 
   // Keep around the last seen summary to be used when we see an optional
   // "OriginalName" attachement.
   GlobalValueSummary *LastSeenSummary = nullptr;
   GlobalValue::GUID LastSeenGUID = 0;
while (true) {
     if (!MaybeBitCode)
       return MaybeBitCode.takeError();
     switch (unsigned BitCode = MaybeBitCode.get()) {
     default: // Default behavior: ignore.
       break;
     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.
       if (Flags & 0x1)
         TheIndex.setWithGlobalValueDeadStripping();
       // 1 bit: SkipModuleByDistributedBackend flag.
       // Set on combined index only.
       if (Flags & 0x2)
         TheIndex.setSkipModuleByDistributedBackend();
       // 1 bit: HasSyntheticEntryCounts flag.
       // Set on combined index only.
       if (Flags & 0x4)
         TheIndex.setHasSyntheticEntryCounts();
       // 1 bit: DisableSplitLTOUnit flag.
       // Set on per module indexes. It is up to the client to validate
       // the consistency of this flag across modules being linked.
       if (Flags & 0x8)
         TheIndex.setEnableSplitLTOUnit();
       // 1 bit: PartiallySplitLTOUnits flag.
       // 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]
       uint64_t ValueID = Record[0];
       GlobalValue::GUID RefGUID = Record[1];
       ValueIdToValueInfoMap[ValueID] =
           std::make_pair(TheIndex.getOrInsertValueInfo(RefGUID), RefGUID);
       break;
while (true) {
     if (!MaybeBitCode)
       return MaybeBitCode.takeError();
     switch (MaybeBitCode.get()) {
     default: // Default behavior: ignore.
       break;
     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;
     }
     }
   }
   llvm_unreachable("Exit infinite loop");
 }
 

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

Stream.EmitRecord(bitc::FS_PERMODULE_VTABLE_GLOBALVAR_INIT_REFS, NameVals,
                       FSModVTableRefsAbbrev);
   }
   NameVals.clear();
 }
 
 // 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.
 void ModuleBitcodeWriterBase::writePerModuleGlobalValueSummary() {
   // By default we compile with ThinLTO if the module has a summary, but the
   // client can request full LTO with a module flag.
   bool IsThinLTO = true;
   if (auto *MD =
void IndexBitcodeWriter::writeCombinedGlobalValueSummary() {
   if (Index.skipModuleByDistributedBackend())
     Flags |= 0x2;
   if (Index.hasSyntheticEntryCounts())
     Flags |= 0x4;
   if (Index.enableSplitLTOUnit())
     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()) {
     Stream.EmitRecord(bitc::FS_VALUE_GUID,
                       ArrayRef<uint64_t>{GVI.second, GVI.first});
   }
 
   // Abbrev for FS_COMBINED.

llvm/lib/IR/ModuleSummaryIndex.cpp

for (auto &S : P.second.SummaryList) {
       // it or any alias (since alias points to the same memory) are preserved
       // or notEligibleToImport, since either of those means there could be
       // writes (or reads in case of writeonly) that are not visible (because
       // preserved means it could have external to DSO writes or reads, and
       // notEligibleToImport means it could have writes or reads via inline
       // 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);
         }
       propagateAttributesToRefs(S.get());
     }
   if (llvm::AreStatisticsEnabled())
     for (auto &P : *this)
       if (P.second.SummaryList.size())
         if (auto *GVS = dyn_cast<GlobalVarSummary>(
                 P.second.SummaryList[0]->getBaseObject()))
           if (isGlobalValueLive(GVS)) {
             if (GVS->maybeReadOnly())
               ReadOnlyLiveGVars++;
             if (GVS->maybeWriteOnly())
               WriteOnlyLiveGVars++;
           }
 }
 
+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
 void ModuleSummaryIndex::dumpSCCs(raw_ostream &O) {
   for (scc_iterator<ModuleSummaryIndex *> I =
            scc_begin<ModuleSummaryIndex *>(this);
        !I.isAtEnd(); ++I) {
     O << "SCC (" << utostr(I->size()) << " node" << (I->size() == 1 ? "" : "s")

llvm/lib/Transforms/IPO/FunctionImport.cpp

updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) {
   // it, rather than needing to perform this mapping on each walk.
   auto GUID = Index.getGUIDFromOriginalID(VI.getGUID());
   if (GUID == 0)
     return ValueInfo();
   return Index.getValueInfo(GUID);
 }
 
 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()) {
     if (DefinedGVSummaries.count(VI.getGUID())) {
       LLVM_DEBUG(
           dbgs() << "Ref ignored! Target already in destination module.\n");
       continue;
     }
 
     LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
 
     // If this is a local variable, make sure we import the copy
     // in the caller's module. The only time a local variable can
     // share an entry in the index is if there is a local with the same name
     // in another module that had the same source file name (in a different
     // directory), where each was compiled in their own directory so there
     // was not distinguishing path.
     auto LocalNotInModule = [&](const GlobalValueSummary *RefSummary) -> bool {
       return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
              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;
       }
   }
 }
 
 static const char *
 getFailureName(FunctionImporter::ImportFailureReason Reason) {
   switch (Reason) {

 /// exported from their source module.
 static void computeImportForFunction(
     const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
     const unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries,
     SmallVectorImpl<EdgeInfo> &Worklist,
     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;
     LLVM_DEBUG(dbgs() << " edge -> " << VI << " Threshold:" << Threshold
                       << "\n");
 
     if (ImportCutoff >= 0 && ImportCount >= ImportCutoff) {
       LLVM_DEBUG(dbgs() << "ignored! import-cutoff value of " << ImportCutoff

 
 // Compute dead symbols and propagate constants in combined index.
 void llvm::computeDeadSymbolsWithConstProp(
     ModuleSummaryIndex &Index,
     const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
     function_ref<PrevailingType(GlobalValue::GUID)> isPrevailing,
     bool ImportEnabled) {
   computeDeadSymbols(Index, GUIDPreservedSymbols, isPrevailing);
   if (ImportEnabled) {
     Index.propagateAttributes(GUIDPreservedSymbols);
   } else {
     // If import is disabled we should drop read/write-only attribute
     // from all summaries to prevent internalization.
     for (auto &P : Index)
       for (auto &S : P.second.SummaryList)
         if (auto *GVS = dyn_cast<GlobalVarSummary>(S.get())) {
           GVS->setReadOnly(false);
           GVS->setWriteOnly(false);
         }
   }
+  Index.setWithAttributePropagation();
 }
 
 /// Compute the set of summaries needed for a ThinLTO backend compilation of
 /// \p ModulePath.
 void llvm::gatherImportedSummariesForModule(
     StringRef ModulePath,
     const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
     const FunctionImporter::ImportMapTy &ImportList,

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   // attribute. We can't internalize them now because IRMover will fail
   // to link variable definitions to their external declarations during
   // ThinLTO import. We'll internalize read-only variables later, after
   // import is finished. See internalizeGVsAfterImport.
   //
   // 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
       // compiled in their respective directories (so the source file name
       // and resulting GUID is the same). Find the one in this module.
       // Handle the case where there is no summary found in this module. That
       // can happen in the distributed ThinLTO backend, because the index only
       // contains summaries from the source modules if they are being imported.
       // We might have a non-null VI and get here even in that case if the name
       // matches one in this module (e.g. weak or appending linkage).
       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");
     }
   }
 
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
       ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
     // Save the original name string before we rename GV below.

llvm/test/Bitcode/summary_version.ll

 ; Check summary versioning
 ; RUN: opt  -module-summary  %s -o - | llvm-bcanalyzer -dump | FileCheck %s
 
 ; CHECK: <GLOBALVAL_SUMMARY_BLOCK
-; CHECK: <VERSION op0=7/>
+; CHECK: <VERSION op0=8/>
 
 
 
 ; Need a function for the summary to be populated.
 define void @foo() {
     ret void
 }

llvm/test/Bitcode/thinlto-deadstrip-flag.ll

 ; REQUIRES: x86-registered-target
 ; RUN: opt -module-summary %s -o %t.o
 
 ; Ensure dead stripping performed flag is set on distributed index
 ; 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"
 
 @glob = global i32 0

llvm/test/Bitcode/thinlto-synthetic-count-flag.ll

 ; REQUIRES: x86-registered-target
 ; RUN: opt -module-summary %s -o %t.o
 
 ; Ensure synthetic entry count flag is not set on distributed index
 ; 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"
 
 @glob = global i32 0

llvm/test/ThinLTO/X86/globals-import.ll

 
 ; RUN: llvm-lto -thinlto-action=import %t1.bc -thinlto-index=%t3.index.bc
 ; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck --check-prefix=IMPORT %s
 ; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t3.index.bc
 ; RUN: llvm-lto -thinlto-action=promote %t2b.bc -thinlto-index=%t3.index.bc
 ; 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() {
 
 ; Second copy of IR object should not have any symbols imported/promoted.
 ; PROMOTE2: @baz = internal constant i32 10, align 4
 ; PROMOTE2: define available_externally i32 @foo() {
 

llvm/test/ThinLTO/X86/local_name_conflict.ll

 ; Test handling when two files with the same source file name contain
 ; static functions/variables with the same name (which will have the same GUID
 ; in the combined index.
 
 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary -module-hash %s -o %t.bc
 ; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict1.ll -o %t2.bc
 ; RUN: opt -module-summary -module-hash %p/Inputs/local_name_conflict2.ll -o %t3.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t4.bc %t.bc %t2.bc %t3.bc
 
 ; This module will import b() which should cause the copy of foo and baz from
 ; 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]]()
 
 ; The copy in %t2.bc should not be exported/promoted/renamed
 ; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=NOEXPORTSTATIC
 ; NOEXPORTSTATIC: @baz = internal constant i32 10, align 4
 ; NOEXPORTSTATIC: define internal i32 @foo()
 

llvm/test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll

 ; RUN: ls %t1.o.imports
 ; RUN: ls %t2.o.imports
 ; RUN: ls %t3.o.imports
 
 ; Regular *thinlto.bc file. "SkipModuleByDistributedBackend" flag (0x2)
 ; 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
 ; "SkipModuleByDistributedBackend" flag (0x2) should be set.
 ; RUN: llvm-bcanalyzer --dump %t2.o.thinlto.bc | FileCheck %s -check-prefixes=CHECK-BC2
 ; CHECK-BC2: <GLOBALVAL_SUMMARY_BLOCK
 ; CHECK-BC2: <FLAGS op0=2/>
 ; CHECK-BC2: </GLOBALVAL_SUMMARY_BLOCK