[PATCH] D19102: Always traverse GlobalVariable initializer when computing the export list

[Teresa Johnson via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

Just needs comment update now that we finally know why gvar init is being imported in this case.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:173
@@ +172,3 @@
+  // the initializer for globals that are referenced. We mimic this behavior
+  // here so that the ExportList is accurate.
+  for (auto &Ref : GVS->refs()) {
----------------
Can you update the comment which is inaccurate since we don't always import initializers? Oh, I just figured out why this is getting imported. It is related to #3 from my earlier list of when materializeInitFor will decide to linkGlobalValueBody():

> 3. New hasLocalLinkage(): We should have promoted any locals referenced by an import before this, so shouldn't be true here

But I was wrong! The one situation where we will not promote a local is a global variable that is constant with unnamed addr:

bool FunctionImportGlobalProcessing::doPromoteLocalToGlobal(
...
  auto *GVar = dyn_cast<GlobalVariable>(SGV);
  if (GVar && GVar->isConstant() && GVar->hasUnnamedAddr())
    return false;

which is exactly what happens in your test case. Can you update the comment accordingly?

Also, maybe add a FIXME that if we had a flag in summary indicating that GVar was constant that we could refine this?


http://reviews.llvm.org/D19102