[PATCH] D55527: Normalize GlobalDecls when used with CPUDispatch

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith marked an inline comment as done.
rsmith added a comment.

Thanks, this makes a lot of sense to me.



================
Comment at: lib/CodeGen/CodeGenModule.cpp:907
+    if (CGM.getTarget().supportsIFunc())
+    Out << ".ifunc";
+  } else if (CGM.getTarget().supportsIFunc())
----------------
Missing indentation on this line.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:910
     Out << ".resolver";
 }
 
----------------
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`?


================
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;
+  }
----------------
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?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2637
+    if (!ImplDecl.getDecl()) {
+      ImplDecl = GD.getWithMultiVersionOrdinal(Ordinal);
 
----------------
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


================
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));
----------------
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.)


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2645-2651
+    } else {
+      if (ImplDecl.getDecl()->getAsFunction()->isDefined())
+        EmitGlobalFunctionDefinition(ImplDecl, nullptr);
+      std::string MangledName = getMangledNameImpl(
+          *this, ImplDecl, ImplDecl.getDecl()->getAsFunction(), false);
+      Func = cast<llvm::Function>(GetGlobalValue(MangledName));
     }
----------------
This too should just be the same `GetAddrOfFunction` call, I think.


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

https://reviews.llvm.org/D55527