[ThinLTO] Simplify attribute propagation

[evgeny777]

Summary

It looks like not calling to canImportGlobalVar during attribute propagation doesn't break any tests. Also, technically speaking, whether or not object can be imported shouldn't affect its readonly property

[tejohnson]

Given the write only bug that popped up with the earlier patch, I'm wondering if this is safe. I.e. we must import a global variable if we have marked it as write only, so if we don't import it later (because it fails one of the legality checks in canImportGlobalVar), then we can end up in trouble.

llvm/include/llvm/IR/ModuleSummaryIndex.h

public:
   bool withGlobalValueDeadStripping() const {
     return WithGlobalValueDeadStripping;
   }
   void setWithGlobalValueDeadStripping() {
     WithGlobalValueDeadStripping = true;
   }
 
   bool withAttributePropagation() const { return WithAttributePropagation; }
-  void setWithAttributePropagation() {
-    WithAttributePropagation = true;
+  void setWithAttributePropagation() { 
+    WithAttributePropagation = true; 
   }
 
   bool isReadOnly(const GlobalVarSummary *GVS) const {
     return WithAttributePropagation && GVS->maybeReadOnly();
   }
   bool isWriteOnly(const GlobalVarSummary *GVS) const {
     return WithAttributePropagation && GVS->maybeWriteOnly();
   }
public:
 
   /// 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;
+  bool canImportGlobalVar(GlobalValueSummary *S) 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) {

llvm/lib/IR/ModuleSummaryIndex.cpp

for (auto &S : P.second.SummaryList) {
       // a local (imported) copy. It also can't be marked read/writeonly if
       // 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. 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)) {
+        if (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 {
+bool ModuleSummaryIndex::canImportGlobalVar(GlobalValueSummary *S) 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.
+  // folding and converting indirect calls to direct calls.
   return !GlobalValue::isInterposableLinkage(S->linkage()) &&
-         !S->notEligibleToImport() &&
-         (!AnalyzeRefs || !HasRefsPreventingImport(GVS));
+         !S->notEligibleToImport() && !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);

llvm/lib/Transforms/IPO/FunctionImport.cpp

for (auto &VI : Summary.refs()) {
 
     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()) &&
-          Index.canImportGlobalVar(RefSummary.get(), /* AnalyzeRefs */ true) &&
+          Index.canImportGlobalVar(RefSummary.get()) &&
           !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++;
         MarkExported(VI, RefSummary.get());
         // Promote referenced functions and variables
         for (const auto &VI : RefSummary->refs())