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

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

rsmith added a comment.

I expect there are more places that need to be changed, but this is looking surprisingly clean.

You will need to teach the modules merging code that it needs to check this attribute in addition to checking that types match when considering merging declaration chains for functions. (See `isSameEntity` in Serialization/ASTReaderDecl.cpp).

I don't see any tests for non-call uses of multiversioned functions (eg, taking their address, binding function references to them). Does that work? (I'd expect you to need "ignore non-default versions" logic in more places for that.)



================
Comment at: include/clang/AST/Decl.h:1754-1756
+  /// Indicates that this function is a multiversioned function using attribute
+  /// 'target'.
+  unsigned IsMultiVersion : 1;
----------------
This flag needs serialization support for PCH / modules, and you'll need to do something if we find declarations with inconsistent values for the flag across modules (detect and diagnose, probably), and likewise in chained PCH (where the right thing to do is presumably to update all prior declarations to have the same value for the flag).


================
Comment at: include/clang/Basic/Attr.td:1809
       bool DuplicateArchitecture = false;
+      bool operator ==(const ParsedTargetAttr &Other) {
+        return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
Our normal convention is to not put a space before `==` here.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9317-9318
+def err_target_required_in_redecl :
+  Error<"function declaration is missing 'target' attribute in a "
+            "multiversioned function">;
+def note_multiversioning_caused_here :
----------------
There's some weird indentation hereabouts. Our normal convention is to put the `Error<` on the `def` line, and start the diagnostic text as a 2-space-indented string literal on the next line.


================
Comment at: include/clang/Basic/TargetInfo.h:927
+  virtual unsigned multiVersionSortPriority(StringRef Name) const {
+    return false;
+  }
----------------
`return 0;`


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2141
+
+    for (auto *CurDecl : FD->getDeclContext()->lookup(FD->getIdentifier())) {
+      if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
----------------
You need to call `getRedeclContext()` on the value returned by `getDeclContext()` to skip "transparent" `DeclContext`s such as `LinkageSpecDecl` and C++ Modules TS `export` declarations.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2141
+
+    for (auto *CurDecl : FD->getDeclContext()->lookup(FD->getIdentifier())) {
+      if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
----------------
rsmith wrote:
> You need to call `getRedeclContext()` on the value returned by `getDeclContext()` to skip "transparent" `DeclContext`s such as `LinkageSpecDecl` and C++ Modules TS `export` declarations.
`getIdentifier` here is wrong; use `getDeclName()` instead, to correctly handle non-identifier names.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+        if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+          StringRef MangledName = getMangledName(CurFD);
+          llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
You should skip functions you've already seen somewhere around here; we don't need to handle the same function multiple times.


================
Comment at: lib/Sema/SemaDecl.cpp:9326
+
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl *OldFD,
+                                             const FunctionDecl *NewFD,
----------------
rsmith wrote:
> Would it be possible to factor out the checks in `MergeFunctionDecl` that you're using here and reuse them directly?
Maybe also check that the language linkage matches (`extern "C"` and `extern "C++"` could imply different calling conventions, even though they don't on any of our current targets).


================
Comment at: lib/Sema/SemaDecl.cpp:9326-9328
+static bool CheckMultiVersionAdditionalRules(Sema &S, const FunctionDecl *OldFD,
+                                             const FunctionDecl *NewFD,
+                                             bool CausesMV) {
----------------
Would it be possible to factor out the checks in `MergeFunctionDecl` that you're using here and reuse them directly?


================
Comment at: lib/Sema/SemaDecl.cpp:9383-9384
+
+    QualType OldReturnType = OldType->getReturnType();
+    QualType NewReturnType = NewType->getReturnType();
+    if (OldReturnType != NewReturnType) {
----------------
This won't do the right thing for C++14 deduced return types. I'm not completely sure what the right thing is, though -- clearly we need to deduce the same return type in all versions of the function, but it's less clear whether we should also require the return-type-as-written to match across versions (as is required across redeclarations). Example:

```
VERSION_FOO auto f() { return 0; } // declared return type is `auto`, actual return type is `int`
VERSION_BAR auto f(); // will fail the current check, because the return type `auto` doesn't match the prior return type `int`
VERSION_BAR auto f() { return 0.0; } // should reject this because we deduced a different return type than on the VERSION_FOO version?
```

Perhaps the simplest thing would be to simply disallow deduced return types for multiversioned functions entirely for now?


================
Comment at: lib/Sema/SemaDecl.cpp:9431-9432
+                                      LookupResult &Previous) {
+  if (NewFD->isMain())
+    return false;
+  const auto *NewTA = NewFD->getAttr<TargetAttr>();
----------------
Should we reject (eg) `__attribute__((target("default")))` on `main`?


================
Comment at: lib/Sema/SemaDecl.cpp:9435
+
+  // If there is no matching previous decl, than only 'default' can
+  // cause MultiVersioning.
----------------
Typo "than"


================
Comment at: lib/Sema/SemaDecl.cpp:9459-9460
+
+  if (OldFD->isMain())
+    return false;
+
----------------
Can this happen if `NewFD` is not `main`?


================
Comment at: lib/Sema/SemaDecl.cpp:9539-9540
+  // previous member of the MultiVersion set.
+  for (NamedDecl *ND : Previous) {
+    auto *CurFD = ND->getAsFunction();
+    if (!CurFD)
----------------
We should be a little careful here: we don't want to allow multiversioning between declarations in different semantic `DeclContext`s:

```
VERSION_DEFAULT void f();
namespace N {
  using ::f;
  VERSION_FOO void f();
}
```

... because when we generate the dispatcher, we won't find the right set of versions. The easiest way to deal with this might be to check near the start of this function whether the semantic `DeclContext` of `CurFD` and `OldFD` are equal, and if not, just bail out and leave the redeclaration merging code to diagnose the conflict. (We already prevent a `DeclContext` from containing two non-overloadable functions where one of them is 'using'd from another `DeclContext` and the other is declared locally.)


https://reviews.llvm.org/D40819