erichkeane added a comment.
Thanks for the feedback @rsmith! I'm working through the lambda issue and a
few other things, but will get to this as soon as I can.
================
Comment at: include/clang/Basic/Attr.td:2031-2042
+ mutable unsigned ActiveArgIndex = 0;
+ void AdvanceActiveArgIndex() const {
+ ++ActiveArgIndex;
+ while(ActiveArgIndex < featuresStrs_size()) {
+ if (std::find(featuresStrs_begin(),
+ featuresStrs_begin() + ActiveArgIndex,
+ *(featuresStrs_begin() + ActiveArgIndex))
----------------
rsmith wrote:
> Sorry, I don't think it's acceptable from a design perspective to have
> mutable state in an `Attr` object, even if you can ensure that only one
> client of the `Attr` will be using the state at a time. CodeGen is going to
> need to track its own index into the attribute's list of clones.
Alright, I'll see if I can figure that out. I should probably do something
similar for the cpu-dispatch, since it actually uses a very similar mechanism.
================
Comment at: include/clang/Basic/AttrDocs.td:1619-1621
+Note that unlike the ``target`` syntax, every option listed creates a new
+version, disregarding whether it is split on a comma inside or outside a
string.
+The following will emit 4 versions of the function.
----------------
rsmith wrote:
> Can we warn on attributes that contain multiple strings where one or more
> such strings contains a comma? That seems like it would always be user error.
>
> I think I'd then prefer to document this as: "The versions can either be
> listed as a comma-separated sequence of string literals or as a single string
> literal containing a comma-separated list of versions. For compatibility with
> GCC, the two formats can be mixed. For example, the following will emit 4
> versions of the function:"
I don't see why not, the warning should be a fairly trivial addition.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:961-970
if (const auto *FD = dyn_cast<FunctionDecl>(ND))
if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
if (FD->isCPUDispatchMultiVersion() || FD->isCPUSpecificMultiVersion())
AppendCPUSpecificCPUDispatchMangling(
CGM, FD->getAttr<CPUSpecificAttr>(), Out);
+ else if (FD->isTargetClonesMultiVersion())
+ AppendTargetClonesMangling(CGM, FD->getAttr<TargetClonesAttr>(), Out);
----------------
rsmith wrote:
> This chain of `if`s and similar things elsewhere make me think we're missing
> an abstraction. Perhaps `FunctionDecl` should have a `getMultiVersionAttr()`
> and/or `getMultiVersionKind()` functions?
I'm not super sure what either buys us... The multiversion attributes are all
somewhat different unfortunately, so they would need to be dispatched
separately later. The 'getMultiVersionKind' is perhaps useful, though its
essentially what isXXXMultiVersion does. I'll think on it, I agree that there
is likely an abstraction somewhere between that can improve this...
================
Comment at: lib/CodeGen/CodeGenModule.cpp:1043
+
+ std::pair<GlobalDecl, unsigned> SpecCanonicalGD{CanonicalGD, VersionID};
----------------
rsmith wrote:
> This target index should be part of the `GlobalDecl`, not tracked alongside
> it, because it identifies which global we're talking about.
I see. I can definitely do that, I was concerned that adding an int to the
GlobalDecl would be an unacceptable increase in size.
================
Comment at: lib/Sema/SemaDecl.cpp:9811-9812
// Main isn't allowed to become a multiversion function, however it IS
// permitted to have 'main' be marked with the 'target' optimization hint.
if (NewFD->isMain()) {
----------------
rsmith wrote:
> Why can't `main` be multiversioned? That seems like an arbitrary restriction.
At the time of implementing 'target', I was unsure of (and still am) how to
accomplish this. It would seem that I'd need to make the entry point a wrapper
that calls the ifunc. GCC seems to improperly call this (it doesn't emit the
'main' fucntion as far as I can tell).
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3068-3071
+ if (!HasDefault) {
+ S.Diag(AL.getLoc(), diag::err_target_clone_must_have_default);
+ return;
+ }
----------------
rsmith wrote:
> Do we need any other validation here? What if there are duplicate versions?
GCC has this annoying feature of just IGNORING duplicate versions. I likely
could/should warn about this, but for GCC compat we probably want to support
this.
================
Comment at: test/CodeGen/attr-cpuspecific.c:14-15
-__attribute__((cpu_specific(ivybridge)))
-void NotCalled(void){}
+__attribute__((cpu_specific(ivybridge))) inline void InlineSingleVersion(void)
{}
+// CHECK: define available_externally void @InlineSingleVersion.S()
#[[S:[0-9]+]]
+
----------------
rsmith wrote:
> Should this really have external linkage? These `.` suffixes are reserved for
> vendor-specific manglings that typically should only be used for symbols with
> internal linkage.
>
> If you really want to give these symbols external linkage, I think they
> should at least be put in a COMDAT keyed off the primary symbol so that we
> don't get a mishmash of different suffix combinations from different
> compilers.
I guess I don't really have a good reason to not just pick up the linkage from
the original. I'll check to see if I can figure out how I got external linkage
in the first place.
https://reviews.llvm.org/D51650
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
