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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
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()) {
----------------
tejohnson wrote:
> 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?
I'll update.
(added a note in our TODO shared doc as well)


http://reviews.llvm.org/D19102