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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D18298#411471, @joker.eph wrote:

> Hitting a wall because I was trying to plumb `const StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>> &
>
>   ModuleToDefinedGVSummaries()` to get the definedGV for the module we import from, but could not because of `void llvm::ComputeCrossModuleImportForModule`.
>
> The latter was introduced in http://reviews.llvm.org/D18945, for which we had some discussions. 
>  I think we're moving in a direction where we can remove this anyway right?


Yes with the changes I am working on to use individual index files for distributed backend compilations, and a change to compute the imports from the gold-plugin and pass them down to the in-process threads, we shouldn't need that anymore. However, I don't think you really need the defined GV info for the module we import from, see below.


================
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:
> > 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.
> > 
> Many tests were in fact broken with this patch as it was before.
Can't you simply reverse this condition? I.e. if it is in the DefinedGVSummaries for the module doing the importing, you aren't going to be externally referencing it by importing the referring function. 

ISTM that it is sufficient to check if var_with_section is not already defined in the importing module, because if not then you are adding a cross-module reference into the importing module. If the symbol in question is not defined in the same module as the function being imported, then presumably it is already externally referenced and canBeExternallyReferenced() should return true anyway. So I think this can just be "if (Summary != DefinedGVSummaries.end()) return true;", and perhaps the name of the routine should change to something like "valueCanBeExternallyReferencedFromImportingModule".


http://reviews.llvm.org/D18298