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

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

rsmith accepted this revision.
rsmith added a comment.

Looks good to me with just a few more tweaks (assuming these comments don't uncover any new issues). Thank you!



================
Comment at: lib/AST/ASTContext.cpp:9490
+  assert(FD->isMultiVersion() && "Only valid for multiversioned functions");
+  llvm::SmallVector<const FunctionDecl*, 4> SeenDecls;
+  FD = FD->getCanonicalDecl();
----------------
Consider using `SmallDenseSet` here. (In the modules case, we could potentially have hundreds or even thousands of lookup results, and the better asymptotics in the bad cases seems on balance worth the higher constant factor.)

Also, you'll need to add declarations to this set :)

A testcase for this would look something like:

```
#pragma clang module build A
module A {}
#pragma clang module contents
#pragma clang module begin A
__attribute__((target("default"))) void f();
__attribute__((target("foo"))) void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module build B
module B {}
#pragma clang module contents
#pragma clang module begin B
__attribute__((target("default"))) void f();
__attribute__((target("foo"))) void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module import A
#pragma clang module import B
void g() { f(); }
```

... with a check that the resolver ifunc only includes each case once.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2832
+
+      if (!TAY || TAX->getFeaturesStr() != TAY->getFeaturesStr())
+        return false;
----------------
The `!TAY` check here is now redundant and can be moved into the assertion.


================
Comment at: test/CodeGen/attr-target-mv.c:20-26
+inline __attribute__((target("default"))) void foo_decls(void);
+inline __attribute__((target("sse4.2"))) void foo_decls(void);
+void bar3() {
+  foo_decls();
+}
+__attribute__((target("default"))) void foo_decls(void) {}
+__attribute__((target("sse4.2"))) void foo_decls(void) {}
----------------
This will either need to be a C++ test or will need `inline` on the definitions to have the intended effect of creating a discardable function definition.


================
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; }
+};
----------------
erichkeane wrote:
> rsmith wrote:
> > 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?
> I gave CTORs a try, and found that there are a few subtle issues that need to be dealt with in code-gen.  For the moment (and since GCC simply terminates if you try to use them), I'd like to disallow them.  
> 
> Definitely going to disallow dtors/default/deleted functions.  
Can you also test that out-of-line definitions of multiversioned member functions are properly matched with the in-class declaration, and that you can't add new versions not present in the class definition that way? (Again, I'm not expecting any problems here, but it seems like something we should have coverage for.)


https://reviews.llvm.org/D40819