[PATCH] D51650: Implement target_clones multiversioning

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

rsmith added inline comments.


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


================
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.
----------------
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:"


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


================
Comment at: lib/CodeGen/CodeGenModule.cpp:1043
+
+    std::pair<GlobalDecl, unsigned> SpecCanonicalGD{CanonicalGD, VersionID};
 
----------------
This target index should be part of the `GlobalDecl`, not tracked alongside it, because it identifies which global we're talking about.


================
Comment at: lib/Sema/SemaDecl.cpp:9800-9807
   // Mixing Multiversioning types is prohibited.
-  if ((NewTA && NewCPUDisp) || (NewTA && NewCPUSpec) ||
-      (NewCPUDisp && NewCPUSpec)) {
+  if ((static_cast<bool>(NewTA) + static_cast<bool>(NewCPUDisp) +
+       static_cast<bool>(NewCPUSpec) + static_cast<bool>(NewTargetClones)) >
+      1) {
     S.Diag(NewFD->getLocation(), diag::err_multiversion_types_mixed);
     NewFD->setInvalidDecl();
     return true;
----------------
We should also disallow multiple `TargetClonesAttr`s on the same declaration. GCC allows this and then ignores all but the last such attribute; we can do better.


================
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()) {
----------------
Why can't `main` be multiversioned? That seems like an arbitrary restriction.


================
Comment at: lib/Sema/SemaDecl.cpp:9839
 
-  if (OldFD->isMultiVersion() && MVType == MultiVersioning::None) {
+  // MultiVersioned redeclarations aren't allowed to omit the attribute except
+  // for target_clones.
----------------
I would expect the plain English term to not have interior capitalization.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3011
 
+bool Sema::checkTargetClonesAttr(SourceLocation LiteralLoc, StringRef Str,
+                                 bool &HasDefault,
----------------
Should be something like `checkTargetClonesAttrStr` to indicate that this is checking one specific string, not the entire attribute.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3018-3029
+  // Warn on empty at the beginning of a string.
+  if (Str.size() == 0 || Str[0] == ',')
+    return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+           << Unsupported << None << "" << TargetClones;
+
+  while (Str.size() != 0) {
+    // Remove the comma we found last time through.
----------------
When looping through comma-delimited strings, it's easier to use `StringRef::split`:

```
std::pair<StringRef, StringRef> Parts = {{}, Str};
while (!Parts.second.empty()) {
  Parts = Parts.second.split(',');
  StringRef Cur = Parts.first.trim();
  // ...
}
```


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3035-3043
+        return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+               << Unsupported << Architecture
+               << Cur.drop_front(sizeof("arch=") - 1) << TargetClones;
+    } else if (Cur == "default") {
+      HasDefault = true;
+      continue;
+    } else if (!Context.getTargetInfo().isValidFeatureName(Cur))
----------------
Rather than just repeating the problematic portion of the string textually in the diagnostic, pass the `StringLiteral` into here and use `StringLiteral::getLocationOfByte` to highlight the problematic source range within the string.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3068-3071
+  if (!HasDefault) {
+    S.Diag(AL.getLoc(), diag::err_target_clone_must_have_default);
+    return;
+  }
----------------
Do we need any other validation here? What if there are duplicate versions?


================
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]+]]
+
----------------
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.


https://reviews.llvm.org/D51650