[PATCH] D51650: Implement target_clones multiversioning

[Erich Keane via Phabricator via cfe-commits]

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