[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

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

rsmith added a comment.

One other test I'd like to see is what happens if you declare a version before the first use, and define it afterwards, particularly if the version is an inline function:

  inline __attribute__((target("default"))) void f();
  inline __attribute__((target("foo"))) void f();
  void g() { f(); }
  __attribute__((target("default"))) void f() {}
  __attribute__((target("foo"))) void f() {}



================
Comment at: include/clang/AST/ASTContext.h:2643-2648
+    for (auto *CurDecl :
+         FD->getDeclContext()->getRedeclContext()->lookup(FD->getDeclName())) {
+      FunctionDecl *CurFD = CurDecl->getAsFunction()->getCanonicalDecl();
+      if (CurFD && hasSameType(CurFD->getType(), FD->getType()))
+        P(CurFD);
+    }
----------------
This should deal with the case where `lookup` finds multiple declarations of the same version (which can happen in particular when serializing to AST files; we keep around a handle to the version from each AST file). Also, consider moving this to the .cpp file (use `llvm::function_ref<void(const FunctionDecl*)>` to pass in the callback).


================
Comment at: include/clang/Basic/Attr.td:1809
       bool DuplicateArchitecture = false;
+      bool operator==(const ParsedTargetAttr &Other) {
+        return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
rsmith wrote:
> aaron.ballman wrote:
> > This function should be marked `const`.
> Nit: space before `{`.
Space before `{` seems to have been removed again. Clang-format really doesn't like tablegen files :)


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9354-9356
+  "multiversioned function declaration has a different %select{calling convention"
+  "|return type|constexpr specification|inline specification|storage class|"
+  "linkage|destructors}0">;
----------------
"multiversioned function declaration has a different destructors" doesn't sound right to me :)


================
Comment at: lib/Sema/SemaDecl.cpp:9228-9229
+    S.Diag(NewFD->getLocation(), diag::err_multiversion_no_other_attrs);
+    if (CausesMV)
+      S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
+    return true;
----------------
It seems strange to have a "caused here" note pointing at the same location that you just produced an error on. Is this note really adding value? (Likewise on all later diagnostics that add this note at the same place as the error.)


================
Comment at: lib/Sema/SemaOverload.cpp:10220
+  case ovl_non_default_multiversion_function:
+    // Do Nothing, these should simly be ignored.
+    break;
----------------
Couple of typos here.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2822
+      const auto *TAY = FuncY->getAttr<TargetAttr>();
+      assert(TAX && TAY && "Multiversion Function without target attribute");
+
----------------
rsmith wrote:
> This assertion will fire if X is multiversion but Y is not. It's probably not too crucial what happens in that case, but we shouldn't assert. An error would be nice, but we can't produce one from here, so how about we just say that if one is multiversion and the other is not, that they're considered to not be the same entity.
This is still not quite right for the case of a multiversion / non-multiversion mismatch: you don't return 'false' when Y is multiversion and X is not.


================
Comment at: test/CodeGenCXX/attr-target-mv-constexpr.cpp:1-16
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+constexpr int __attribute__((target("sse4.2"))) foo(void) { return 0; }
+constexpr int __attribute__((target("arch=sandybridge"))) foo(void);
+constexpr int __attribute__((target("arch=ivybridge"))) foo(void) {return 1;}
+constexpr int __attribute__((target("default"))) foo(void) { return 2; }
+
----------------
Can you move this to test/SemaCXX and change it to only test the constant evaluation side of things (eg, `static_assert(foo() == 2)`)?


https://reviews.llvm.org/D40819