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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 3 19:01:34 PST 2018


rsmith added a comment.

Lots of comments, but no high-level design concerns. I think this is very close to being ready to go.



================
Comment at: include/clang/AST/Decl.h:2162-2168
+  /// Sets the multiversion state for this declaration and all of its
+  /// redeclarations.
+  void setIsMultiVersion(bool V = true) {
+    IsMultiVersion = V;
+    for (FunctionDecl *FD : redecls())
+      FD->IsMultiVersion = V;
+  }
----------------
Instead of looping over redeclarations here, how about only storing the flag on the canonical declaration (and only looking there in `isMultiVersion`)?


================
Comment at: include/clang/Basic/Attr.td:1809
       bool DuplicateArchitecture = false;
+      bool operator==(const ParsedTargetAttr &Other) {
+        return DuplicateArchitecture == Other.DuplicateArchitecture &&
----------------
aaron.ballman wrote:
> This function should be marked `const`.
Nit: space before `{`.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9319-9320
+def err_target_required_in_redecl : Error<
+  "function declaration is missing 'target' attribute in a multiversioned "
+  "function">;
+def note_multiversioning_caused_here : Note<
----------------
Is it "multiversion function" or "multiversioned function"? You're using both in these diagnostics; please pick one and use it consistently. I prefer the "-ed" form, but I'm happy with whichever you'd prefer.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9322
+def note_multiversioning_caused_here : Note<
+  "function multiversion caused by this declaration">;
+def err_multiversion_after_used : Error<
----------------
"multiversioning" maybe? (Again, this is used inconsistently; there's a "function multiversioning" in err_multiversion_not_supported).


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9329-9331
+def note_ovl_candidate_nondefault_multiversion : Note<
+  "candidate ignored: non-default multiversion function cannot be called "
+  "directly">;
----------------
Maybe we should just suppress the "candidate" note entirely for these cases?


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2144
+        if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
+          StringRef MangledName = getMangledName(CurFD);
+          llvm::Constant *Func = GetGlobalValue(MangledName);
----------------
erichkeane wrote:
> rsmith wrote:
> > You should skip functions you've already seen somewhere around here; we don't need to handle the same function multiple times.
> Will the lookup call give me duplicates?  I need to check each at least once through this so that I can put them into "Options" so that I can emit them during the resolver.  
> 
> Otherwise, I'm not terribly sure what you mean.
Yes, the lookup can in some cases find multiple declarations from the same redeclaration chain. (This happens particularly when the declarations are loaded from AST files.)


================
Comment at: lib/CodeGen/CodeGenModule.cpp:840-841
+
+  const auto *ND = cast<NamedDecl>(GD.getDecl());
+  UpdateMultiVersionNames(GD, ND);
+
----------------
I'm not especially enamoured with `getMangledName` mutating the IR. Can we perform this rename as part of emitting the multiversion dispatcher or ifunc, rather than here? (Or do we really not have anywhere else that this can live?)


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2056-2057
   const auto *F = cast<FunctionDecl>(GD.getDecl());
+  if (F->isMultiVersion())
+    return true;
   if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
----------------
Please add a comment explaining this check. (I'm guessing the issue is that we can't form a cross-translation-unit reference to the right function from an IFUNC, so we can't rely on an available_externally definition actually being usable? But it's not clear to me that this is the right resolution to that, especially in light of the dllexport case below where it would not be correct to emit the definition.)


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2140-2143
+    for (auto *CurDecl : FD->getDeclContext()->getRedeclContext()->lookup(
+             FD->getDeclName())) {
+      if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
+        if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
----------------
Consider moving the lookup, loop and type check here into an ASTContext or FunctionDecl utility to find or visit all the versions of a multiversioned function.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:2142
+             FD->getDeclName())) {
+      if (const auto *CurFD = dyn_cast<FunctionDecl>(CurDecl))
+        if (getContext().hasSameType(CurFD->getType(), FD->getType())) {
----------------
`if` statements with nontrivial bodies should have their own braces.


================
Comment at: lib/Sema/SemaDecl.cpp:9526
+      S.Diag(NewFD->getLocation(), diag::err_multiversion_not_supported);
+      S.Diag(OldFD->getLocation(), diag::note_previous_decl) << "previously";
+      return true;
----------------
Do not hardcode English text as a diagnostic parameter. And don't reuse a diagnostic to mean something else :) The %0 in this diagnostic is for the name of the declaration.

`diag::note_previous_declaration` seems to capture what you're actually looking for here. (It's a note for "previous declaration of the same entity", whereas `note_previous_decl` is a note for "here's some previous declaration of something else that's relevant").


================
Comment at: lib/Sema/SemaDecl.cpp:9531-9532
+    if (!NewFD->getType()->getAs<FunctionProtoType>()) {
+      S.Diag(OldFD->getLocation(), diag::err_multiversion_noproto);
+      S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
+      NewFD->setInvalidDecl();
----------------
This doesn't seem right: the OldFD isn't (necessarily) a declaration without a prototype.


================
Comment at: lib/Sema/SemaDecl.cpp:9553
+      S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
+      S.Diag(OldFD->getLocation(), diag::note_previous_decl) << "previously";
+      NewFD->setInvalidDecl();
----------------
As above.


================
Comment at: lib/Sema/SemaDecl.cpp:9607
+      S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
+      S.Diag(CurFD->getLocation(), diag::note_previous_decl) << "previously";
+      NewFD->setInvalidDecl();
----------------
As above.


================
Comment at: lib/Sema/SemaDecl.cpp:9720
+  if (CheckMultiVersionFunction(*this, NewFD, Redeclaration, OldDecl,
+                                MergeTypeWithPrevious, Previous))
+    return Redeclaration;
----------------
The parameter in `CheckMultiVersionFunction` corresponding to `MergeTypeWithPrevious` here is called `MayNeedOverloadableChecks`, which is also the name of a local variable in this function. Did you pass the wrong bool? Or is the name of the parameter to `CheckMultiVersionFunction` wrong?


================
Comment at: lib/Sema/SemaOverload.cpp:5944
+  if (Function->isMultiVersion() &&
+      Function->getAttr<TargetAttr>()->getFeaturesStr() != "default") {
+    Candidate.Viable = false;
----------------
Please move this comparison of `getFeaturesStr()` against "default" into a dedicated function on `TargetAttr` (`::isDefaultVersion()`?). This magic string literal is appearing quite a lot in this patch...


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2818
     }
+    // Multiversioned functions with different 
+    if (FuncX->isMultiVersion()) {
----------------
This comment is missing a


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2822
+      const auto *TAY = FuncY->getAttr<TargetAttr>();
+      assert(TAX && TAY && "Multiversion Function without target attribute");
+
----------------
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.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2824
+
+      if(TAX->getFeaturesStr() != TAY->getFeaturesStr())
+        return false;
----------------
Space after `if`, please.


================
Comment at: test/CodeGen/attr-target-mv-func-ptrs.c:10-15
+int bar() {
+  func(foo);
+  FuncPtr Free = &foo;
+  FuncPtr Free2 = foo;
+  return Free(1) + Free(2);
+}
----------------
What about uses in contexts where there is no target function type? For example, `+foo;`


================
Comment at: test/CodeGenCXX/attr-target-mv-member-funcs.cpp:3-8
+struct S {
+  int __attribute__((target("sse4.2"))) foo(int) { return 0; }
+  int __attribute__((target("arch=sandybridge"))) foo(int);
+  int __attribute__((target("arch=ivybridge"))) foo(int) { return 1; }
+  int __attribute__((target("default"))) foo(int) { return 2; }
+};
----------------
OK, multiversioned member functions! Let's look at some nasty corner cases!

Do you allow multiversioning of special member functions (copy constructor, destructor, ...)? Some tests for that would be interesting. Note in particular that `CXXRecordDecl::getDestructor` assumes that there is only one destructor for a class, and I expect we make that assumption in a bunch of other places too. Might be best to disallow multiversioning destructors for now.

Do you allow a multiversioned function to have one defaulted version and one non-defaulted version? What does that mean for the properties of the class that depend on whether special member functions are trivial? Might be a good idea to disallow defaulting a multiversioned function. Oh, and we should probably not permit versions of a multiversioned function to be deleted either.

If you allow multiversioning of constructors, I'd like to see a test for multiversioning of inherited constructors. Likewise, if you allow multiversioning of conversion functions, I'd like to see a test for that. (I actually think there's a very good chance both of those will just work fine.)

You don't allow multiversioning of function templates right now, but what about multiversioning of member functions of a class template? Does that work? If so, does class template argument deduction using multiversioned constructors work?

Does befriending multiversioned functions work? Is the target attribute taken into account?


https://reviews.llvm.org/D40819





More information about the cfe-commits mailing list