[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:39
@@ +38,3 @@
+; PROMOTE-DAG: @linkonceODRfuncWeakAlias = weak alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
+; PROMOTE-DAG: @linkonceODRfuncLinkonceAlias = linkonce alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
+; PROMOTE-DAG: @linkonceODRfuncWeakODRAlias = weak_odr alias void (...), bitcast (void ()* @linkonceODRfunc to void (...)*)
----------------
tejohnson wrote:
> joker.eph wrote:
> > tejohnson wrote:
> > > joker.eph wrote:
> > > > There is a bug I think, this one should be promoted right?
> > > You mean resolved? The alias itself isn't ODR so I think it stays Linkonce(Any).
> > I think the current situation is murky. Let see if I can summarize it right:
> > 
> > - linkonce (ODR or not) can be discarded: if the symbol is referenced from another module, it needs to be "resolved" for *correctness*.
> > - weak (ODR or not) can't be discarded: we don't need to resolve them for *correctness*.
> > - linkonce/weak (the *non* ODR variant) can't be inlined, there is probably no interest in importing them, we should disable that in the importing decision.
> > - For all of them: we want to resolve when there is more than one definition for compile-time optimization purpose.
> Yes, I think that is a correct summary. So the bug as you point out is in fact that we should be resolving linkonce(any) to weak(any) since it is discardable.
> 
> Otherwise if under some situation the call was removed in the original exporting module (not through inlining since that doesn't happen for non-ODR linkonce, but some other transformation), any imported reference that didn't pull in the def can get a linker undef.
> 
> So seems we need to do the resolving of linkonce(any) to weak(any) if it is exported. And could do it when there is more than one definition as a compile time optimization.
> 
> I remember that in an older version of the patch you were doing this resolution for the non-odr types as well, but I don't remember why you stopped doing that for the WeakAny and LinkOnceAny. Was there an issue that I am not remembering?
I don't remember the history unfortunately.

What I am gonna do is moving forward with this patch, leaving the bug as-is, since it is already existing I think.
I'll implement in a new patch (or a few patches) the proper handling of the 4 points above.


http://reviews.llvm.org/D19308