[ThinLTO] Add option to disable readonly/writeonly attribute propagation

[tejohnson]

Summary

Add an option to allow the attribute propagation on the index to be
disabled, to allow a workaround for issues (such as that fixed by
D70977). Restructure the code to include all attribute propagation
handling into propagateAttributes() itself (rather than having some in
the caller), which also simplifies handling of the new option.

[steven_wu]

llvm/lib/IR/ModuleSummaryIndex.cpp

166

Do we still need to drop all the readonly writeonly flag if simply return before setWithAttributePropagation()?

[tejohnson]

llvm/lib/IR/ModuleSummaryIndex.cpp

166

Good point, this code predated having the WithAttributePropagation flag on the index, which gates its use. I stripped this stale code out, and moved the check of the ImportEnabled back to the caller. I am still moving the setting of the WithAttributePropagation flag into propagateAttributes where I think it makes more sense.

[tejohnson]

Address comments

[steven_wu]

LGTM!

llvm/lib/IR/ModuleSummaryIndex.cpp

 // IR library.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/ADT/SCCIterator.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "module-summary-index"
 
 STATISTIC(ReadOnlyLiveGVars,
           "Number of live global variables marked read only");
 STATISTIC(WriteOnlyLiveGVars,
           "Number of live global variables marked write only");
 
+static cl::opt<bool> PropagateAttrs("propagate-attrs", cl::init(true),
+                                    cl::Hidden,
+                                    cl::desc("Propagate attributes in index"));
+
 FunctionSummary FunctionSummary::ExternalNode =
     FunctionSummary::makeDummyFunctionSummary({});
 
 bool ValueInfo::isDSOLocal() const {
   // Need to check all summaries are local in case of hash collisions.
   return getSummaryList().size() &&
          llvm::all_of(getSummaryList(),
                       [](const std::unique_ptr<GlobalValueSummary> &Summary) {

 //
 //   Because of (c, d) we don't internalize variables read by function A
 //   and modified by function B.
 //
 // Internalization itself happens in the backend after import is finished
 // See internalizeGVsAfterImport.
 void ModuleSummaryIndex::propagateAttributes(
     const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
+  if (!PropagateAttrs)
+    return;
   for (auto &P : *this)
     for (auto &S : P.second.SummaryList) {
       if (!isGlobalValueLive(S.get()))
         // We don't examine references from dead objects
         continue;
 
       // Global variable can't be marked read/writeonly if it is not eligible
       // to import since we need to ensure that all external references get
for (auto &S : P.second.SummaryList) {
         // 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());
     }
+  setWithAttributePropagation();
   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++;

llvm/lib/Transforms/IPO/FunctionImport.cpp

 
 // 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) {
+  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/test/ThinLTO/X86/writeonly.ll

 ; Check that we've optimized out variables and corresponding stores
 ; OPTIMIZE-NOT:  gFoo
 ; OPTIMIZE-NOT:  gBar
 ; OPTIMIZE:      i32 @main
 ; OPTIMIZE-NEXT:   %1 = tail call i32 @rand()
 ; OPTIMIZE-NEXT:   %2 = tail call i32 @rand()
 ; OPTIMIZE-NEXT:   ret i32 0
 
+; Confirm that with -propagate-attrs=false we no longer do write-only importing
+; RUN: llvm-lto -propagate-attrs=false -thinlto-action=import -exported-symbol=main  %t1.bc -thinlto-index=%t3.index.bc -o %t1.imported.bc -stats 2>&1 | FileCheck %s --check-prefix=STATS-NOPROP
+; RUN: llvm-dis %t1.imported.bc -o - | FileCheck %s --check-prefix=IMPORT-NOPROP
+; STATS-NOPROP-NOT: Number of live global variables marked write only
+; IMPORT-NOPROP: @gFoo.llvm.0 = available_externally
+; IMPORT-NOPROP-NEXT: @gBar = available_externally
+
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 @gBar = external global i32
 
 ; Should not be counted in the stats of live write only variables since it is
 ; dead and will be dropped anyway.
 @gDead = internal unnamed_addr global i32 1, align 4
 
 define i32 @main() local_unnamed_addr {
   tail call void @baz()
   ret i32 0
 }
 declare void @baz() local_unnamed_addr