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