[PATCH] D18298: ThinLTO: do not promote GlobalVariable that have a specific section.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:99
@@ +98,3 @@
+  auto Summary = DefinedGVSummaries.find(GUID);
+  if (Summary == DefinedGVSummaries.end())
+    return true;
----------------
joker.eph wrote:
> tejohnson wrote:
> > DefinedGVSummaries only contains entries for the module we are importing into, not from. So I don't think this is the right check. In fact, won't this cause this routine to always return true for anything not already in this module and therefore exactly when we would be introducing a cross-module reference?
> > 
> > Looks like your new test is checking that we don't import a function that has a section, but needs to be extended to check that we don't import a function containing references to something that has a section.
> You're right that the logic seems wrong.
> 
> But I'm not sure your are describing what the test is doing: it is checking that we don't import `reference_gv_with_section` which is a function that does not have a section attached, but reference an internal variable with a section.
You're right, I looked too quickly at the test to see why it didn't fail. I'm wondering why it doesn't import reference_gv_with_section (incorrectly), since I think valueCanBeExternallyReferencedFromModule should return true since var_with_section wouldn't be in the DefinedGVSummaries for the importing module.



http://reviews.llvm.org/D18298