[PATCH] D67058: [clang][CodeGen] Add alias for cpu_dispatch function with IFunc

[Sr.Zhang via Phabricator via cfe-commits]

zsrkmyn added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2957
+    if (!AliasFunc) {
+      auto *IFunc = cast<llvm::GlobalIFunc>(GetOrCreateLLVMFunction(
+          AliasName, DeclTy, GD, /*ForVTable=*/false, /*DontDefer=*/true,
----------------
erichkeane wrote:
> erichkeane wrote:
> > zsrkmyn wrote:
> > > erichkeane wrote:
> > > > erichkeane wrote:
> > > > > I think we want this in GetOrCreateMultiVersionResolver, so that it gets created when the ifunc does.  That way you just need a "FD->isCPUDispatchMultiVersion() || isCPUSpecificMultiVersion()" check inside the supportsIFunc branch.
> > > > After discussing this offline, I believe this is the right function to create the alias.  The motivating example is:
> > > > 
> > > >     // TU1:
> > > >     __attribute__((cpu_dispatch(a,b,c))) void foo(void);
> > > > 
> > > >     // TU2:
> > > >     extern void foo(void);
> > > > 
> > > > Currently, TU1 doesn't bother to emit the ifunc, because we've attached emitting this to when this is referenced.
> > > > 
> > > > We made that choice because we expected TU2 to mark 'foo' as cpu_dispatch/cpu_specific in SOME way.  I believe that it is harmless to emit the ifunc all the time, which this is attempting to do.  However, this needs to change the ifunc to have LinkOnceODR linkage in GetOrCreateMultiVersionResolver, otherwise this can cause linker errors.
> > > > 
> > > > 
> > > I've finished most parts. But I think we should also set the resolver to have LinkOnceODR Linkage. Otherwise, we cannot have the cpu_dispatch declaration in multiple TUs.
> > > 
> > > However there's no 'weak' symbol on Windows, so even setting the resolver linkage to LinkOnceODR cannot solve the duplicate defined symbol problem on Windows. Do you have any suggestions on it? :-)
> > Yep, I think the resolver should also be linkonceodr (as well as the ifunc).  See where they are generated and do it there.
> > 
> > I can't help but think that we've solved the weak symbols issue with windows before, but I cannot for the life of me remember.  @rnk  @lebedev.ri , do either of you remember?  Does LinkOnceODR not do that trickery?
> There must be SOME solution for it, since otherwise inline functions wouldn't work.  For example:
> 
> https://godbolt.org/z/RjB9Xb
> 
> Its totally permissible to define and use 'foo::bar' in multiple TUs.  Note that it is marked linkonce_odr dso_local and comdat.  I'm not sure what the latter two do, but please experiment and see which will allow the symbol to be merged in the linker.
Thanks Erich! Yes, It's the comdat that does the trick. I'll update the patch later. https://llvm.org/docs/LangRef.html#comdats


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67058/new/

https://reviews.llvm.org/D67058