[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:86
@@ +85,3 @@
+  if (Summary.hasSection())
+    // Can't rename a globals that needs renaming if has a section
+    return false;
----------------
s/globals/global/

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:99
@@ +98,3 @@
+  auto Summary = DefinedGVSummaries.find(GUID);
+  if (Summary == DefinedGVSummaries.end())
+    return true;
----------------
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.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:106
@@ +105,3 @@
+
+///
+static bool
----------------
Empty comment.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:113
@@ +112,3 @@
+  if (!canBeExternallyReferenced(Summary))
+    // Can't import a globals that needs renaming if has a section for instance.
+    return false;
----------------
s/globals/global/

Can you add a FIXME here that we could import this if we do so as a copy (e.g. don't promote an imported local function). And that if we do so we could also import functions containing references to it?


http://reviews.llvm.org/D18298