[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:
> joker.eph wrote:
> > tejohnson wrote:
> > > joker.eph wrote:
> > > > tejohnson wrote:
> > > > > 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".
> > > > I'm not sure to understand (but haven't had my coffee this morning yet...).
> > > > 
> > > > If you look at the test, we want to import function `@reference_gv_with_section`  which references `@var_with_section` which is internal (so can't be externally referenced). And this is what I'm checking. 
> > > > 
> > > > Can you illustrate the logic you're explaining above using the test as an example?
> > > Sure.
> > > 
> > > When we invoke eligibleForImport on @reference_gv_with_section, it will first check whether its own summary canBeExternallyReferenced() which is true (@reference_gv_with_section is not itself a local that has a section), so then it invokes this routine to see whether the referenced @var_with_section can be externally referenced. Because @var_with_section is not defined in the module we are importing into, it is not in the DefinedGVSummaries list. So currently with your patch, valueCanBeExternallyReferencedFromModule will incorrectly return true since it isn't in the list. With my proposed change, which is:
> > >    if (Summary != DefinedGVSummaries.end())
> > >       return true;
> > > we will not return true here, since the summary for @var_with_section is not in the list (not defined in the importing module). Then it will fall through to check canBeExternallyReferenced(), which should return false given the that it is local and therefore needsRenaming(), and because it hasSection flag.
> > > 
> > Interestingly it seemed obvious when I reached the first parenthesis of the first sentence now. I don't know why I went with something so complicated...
> > Thanks.
> Somehow you convinced me above, but I'm confused again now. What you write is almost what I attempted originally until I wrote that I hit a wall and went for the current patch.
> 
> It is not clear to me how you check anything on `var_with_section ` without its summary. And you will *not* find it if `DefinedGVSummaries` is about the importing module and not the exporting one.
Oh I see your concern - because that is how you are getting the summary to check. Why don't you just get it the same way selectCallee gets it for a candidate call? I.e. access it from the Index via Index.findGlobalValueInfoList(GUID). If there is more than one then you could look for the one in the same module as the referring summary (which would need to be passed in here). That's where you could return false if none is found in the referring summary's module (since it already must be an external access).


http://reviews.llvm.org/D18298