[PATCH] D19308: ThinLTO: Resolve linkonce_odr aliases just like functions

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: test/ThinLTO/X86/alias_resolution.ll:8
@@ +7,3 @@
+
+; Do not resolve all these aliases
+; PROMOTE-DAG: @globalfuncAlias = alias void (...), bitcast (void ()* @globalfunc to void (...)*)
----------------
joker.eph wrote:
> tejohnson wrote:
> > maybe add ", since there is a single copy of each, and they are not exported and discardable" for clarity
> The "since there is a single copy of each" part seems irrelevant: this is really about are they exported or not. 
> And we only import alias when they point to a linkonce_odr function, so these are not exported and won't need to be resolved (I'm not sure if this limitation has any other source than being inherited from the old ModuleLinker).
> 
The limitation is due to the linkage changes made when we do the importing, isn't it related to this comment in the function importer?:

+        // Alias can't point to "available_externally". However when we import
+        // linkOnceODR the linkage does not change. So we import the alias
+        // and aliasee only in this case.
+        // FIXME: we should import alias as available_externally *function*, the
+        // destination module does need to know it is an alias.

================
Comment at: test/ThinLTO/X86/alias_resolution.ll:35
@@ +34,3 @@
+
+; Only alias to LinkonceODR function are resolved
+; (This is a current limitation of the implementation that could be lifted)
----------------
joker.eph wrote:
> tejohnson wrote:
> > Shouldn't this be:
> > "Only linkonce_odr alias is resolved in the case when there is a single copy"
> > I guess this depends on what it means to be "resolved". I took "resolved" to mean we decided which copy to select, which isn't the case for most aliases.
> Resolved in the sense of the function `ResolveODR`:
> 
> ```
> /// We'd like to drop these function if they are no longer referenced in the
> /// current module. However there is a chance that another module is still
> /// referencing them because of the import. We make sure we always emit at least
> /// one copy.
> ```
> 
> So it is about the aliases being linkonce *and* being exported, that needs to be turned into weak(_odr).
> 
> Are we talking about the same "resolve"?
My point was that some of the above linkonce_odr aliases would also be "resolved" to weak_odr by ResolveODR if there were multiple copies, and they were the first definition for linker. I.e. in this test, the reason why we resolve the below linkonce_odr alias and not the earlier ones is due to the fact that there is a single copy (which is exported).

Also, we're not really resolving anything other than the ODR aliases - the rest are ignored by ResolveODR. So the non linkonce_odr/weak_odr aliases aren't really being resolved are they?

================
Comment at: test/ThinLTO/X86/alias_resolution.ll:50
@@ +49,3 @@
+
+; Only import aliases that have been resolved, i.e. alias to a linkonce_odr.
+; IMPORT-NOT:  @linkonceODRfuncWeakAlias = weak alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
----------------
joker.eph wrote:
> tejohnson wrote:
> > The word "resolved" here seems contradictory to how it is being used in the rest of the patch. Maybe change this to something like:
> > " Only import aliases that will not become available_externally, i.e. alias to a linkonce_odr."
> I should write "The current implementation is limited (for no real reasons) to aliases to linkonce_odr", unless there is something I missed...
> 
This is related to the comment I copied in earlier, it is due to the need to replace alias with copy when importing, due to needing to avoid an alias to available_externally, right? I was suggesting that the reason be documented more specifically in the test (we will want to change the test when we can import more aliases).


http://reviews.llvm.org/D19308