[PATCH] D55527: Normalize GlobalDecls when used with CPUDispatch

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 7 inline comments as done.
erichkeane added a comment.

I got a couple of @rsmith's requests done.  Most importantly I suspect is `MultiVersionFuncs`, though `NotForDefinition` vs `ForDefinition` is perhpas something you'll find important.



================
Comment at: lib/CodeGen/CodeGenModule.cpp:910
     Out << ".resolver";
 }
 
----------------
erichkeane wrote:
> 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.  
I thought about it more last night, if the purpose is simply unique-ness, we can generate it as a unique name.  The intent is that it should never make it into the IR, so I think appending the names and .ifunc is fine, simply so that it is unique.


================
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;
+  }
----------------
erichkeane wrote:
> 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?  
I tried for a bit, and couldn't come up with a way that this would work for `Target` as well.  IMO, since `MultiVersionFuncs` already has to exist, this fits better there.

Additionally, I noticed that when I switched Dispatch over to this version that I was having to emit more functions during resolver emission, since they were in the deferred list.  The deferred list then attempted to emit them, but no longer needed to (though no problems were caused by this).  


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2637
+    if (!ImplDecl.getDecl()) {
+      ImplDecl = GD.getWithMultiVersionOrdinal(Ordinal);
 
----------------
erichkeane wrote:
> 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.
I rephrased this slightly and put it in "EmitMultiVersionFunctionDefinition", since it seems to fit there best IMO.


================
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));
----------------
erichkeane wrote:
> 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.
I replaced both of these as you stated, though I DID need to do `ForDefinition`.  I couldn't come up with a not-super-intrusive way to differentiate between "Not for definition, but for resolver use" and "Not for definition, give me the IFunc".


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

https://reviews.llvm.org/D55527