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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
