[PATCH] D26880: [ThinLTO] Fix crash: transitively referenced global shouldn't be promoted

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26880#600928, @mehdi_amini wrote:

> > I think you would also need this patch to go in so globalvar refs wouldn't then be marked exported, which presumably is causing an issue when the exporting (defining) module tried to promote those with sections?
>
> Yes, this is the crash with the test-case in this patch.
>
> >> - work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)
> > 
> > This is the right long term fix. However I am concerned about performance we may give up in the meantime (PR31052 is  fixing a case where we already are getting optimized code). I'd prefer to keep the current approach in the meantime, unless benchmarking shows that it doesn't have significant impact. It would mean changing this fix to instead simply do a recursive walk of global var refs in eligibleForImport, along with my current fix for PR31052.
>
> I have a different approach: 1) this should go in in any case, because correctness first,


This patch will not be correct on it's own, it can't go in without removing the logic in doPromoteLocalToGlobal.

> and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.

I'm concerned about the performance we lose in the meantime. Note the optimization is working in other cases (when the reference to a non-const local is directly referenced by the const local being imported). To keep it working until the constness is tracked and handled in the thin link, along with my patch for PR31052, I think this would need to be changed to do a similar recursive check in canBeExternallyReferenced, like the following:

- a/lib/Transforms/IPO/FunctionImport.cpp

+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -135,7 +135,23 @@ static bool canBeExternallyReferenced(const ModuleSummaryIndex &Index,

  // We don't need to check for the module path, because if it can't be
  // externally referenced and we call it, it is necessarilly in the same
  // module

- return canBeExternallyReferenced(**Summaries->second.begin());

+  if (!canBeExternallyReferenced(**Summaries->second.begin()))
+    return false;
+
+  auto GVS = dyn_cast<GlobalVarSummary>(Summaries->second.begin()->get());
+  if (!GVS)
+    return true;
+  // FunctionImportGlobalProcessing::doPromoteLocalToGlobal() will always
+  // trigger importing  the initializer for `constant unnamed addr` globals that
+  // are referenced. We conservatively check all the referenced symbols for
+  // every global to workaround this.
+  // FIXME: with a "isConstant" flag in the summary we could be more targetted.
+  for (auto &Ref : GVS->refs()) {
+    auto GUID = Ref.getGUID();
+    if (!canBeExternallyReferenced(Index, GUID))
+      return false;
+  }
+  return true;
 }

// Return true if the global described by \p Summary can be imported in another


https://reviews.llvm.org/D26880