[PATCH] D55527: Normalize GlobalDecls when used with CPUDispatch

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 4 inline comments as done.
erichkeane added inline comments.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:910
     Out << ".resolver";
 }
 
----------------
rsmith wrote:
> Hmm, it looks like we don't have a unique name for a `GlobalDecl` naming the `CPUOrdinal == 0` case of an overloaded `cpu_specific` function:
> ```
> __attribute__((cpu_specific(atom)))
> void f() {} // CPUOrdinal 0 GD mangled as _Z1fv.ifunc
> __attribute__((cpu_specific(haswell)))
> void f() {} // CPUOrdinal 0 GD also mangled as _Z1fv.ifunc
> ```
> Maybe we could append the CPU-specific manglings for all named CPUs before the `.ifunc`?
Well shoot, thats true, since you can call the function at different points and have overload resolution 'choose' a different one (since it is just by ordering).

If we were to give it a different name, we would either have to replace it later, or make sure a call to 'not the ifunc' doesn't happen.  


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2133-2138
+  // If this is a cpu_dispatch multiversion function, designate it for emission
+  // at the end of the Translation Unit.
+  if (Global->hasAttr<CPUDispatchAttr>()) {
+    MultiVersionFuncs.push_back(GD);
+    return;
+  }
----------------
rsmith wrote:
> Instead of adding a special-case list of `MultiVersionFuncs`, could we instead change `MayBeEmittedEagerly` to return `false` for `CPUDispatch` functions and use the normal `DeferredDecls` mechanism for this?
That list already would have to exist for GCC Multiversioning, since there isn't a single declaration that corresponds to the resolver.  I could definitely remove CPUDispatch from this mechanism and return it to a separate one, but I'm not sure what that buys us.

Unless you think this is something we can do that will fix the GCC Multiversioning as well?  


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2637
+    if (!ImplDecl.getDecl()) {
+      ImplDecl = GD.getWithMultiVersionOrdinal(Ordinal);
 
----------------
rsmith wrote:
> I think we should have a comment somewhere explaining the subtle use of the multiversion ordinal:
> 
>  * (Resolver, 0) refers to the resolver itself
>  * (Resolver, N) refers to an external (outside this module) symbol for the N'th CPU in Resolver's cpu_dispatch list
>  * (Specific, 0) is a placeholder used to refer to the set of function symbols defined by a cpu_specific function declaration; no corresponding symbol is ever emitted (right?)
>  * (Specific, N) refers to an internal (within this module) symbol for the N'th CPU in Specific's cpu_specific list
(Specific,0) is a placeholder for a not-yet-existing CPU-Dispatch version of the function.  Otherwise, I think documenting that somewhere is a good idea.  I'll look for a good spot for it.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2639-2644
+      std::string MangledName = getMangledNameImpl(*this, ImplDecl, FD, true) +
+                                getCPUSpecificMangling(*this, II->getName());
+      Func = cast<llvm::Function>(GetOrCreateLLVMFunction(
+          MangledName, DeclTy, ImplDecl,
           /*ForVTable=*/false, /*DontDefer=*/true,
+          /*IsThunk=*/false, llvm::AttributeList(), ForDefinition));
----------------
rsmith wrote:
> I think the first part of this should now be equivalent to `getMangledName(...ImplDecl...)`, which should means that these 6 lines can be replaced by `GetAddrOfFunction(ImplDecl, DeclTy, false, false, NotForDefinition)`.
> 
> (Also I think you should pass `NotForDefinition` here because you just want to reference the symbol, you don't want to define it yourself.)
I remember 'DontDefer=true' being set for a good reason, since we need this function emitted.

At the moment, this doesn't work because GetOrCreateLLVMFunction (called by GetAddressOfFunction) replaces NotForDefinition with the ifunc instead of the function, so calling that here ends up getting the ifunc/resolver instead of the function itself.  

I'll continue looking into that, but if you have alternative design decisions, I'd love to hear them.


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

https://reviews.llvm.org/D55527