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

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
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)
----------------
tejohnson wrote:
> 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?
You're right!

It means I miss a test to check that with multiple aliases, even without import they are resolved. This is purely "resolve on import".




================
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 (...)*)
----------------
tejohnson wrote:
> 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).
Yes


http://reviews.llvm.org/D19308