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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 34 inline comments as done.
erichkeane added a comment.

Patch incoming, sorry it took so long!



================
Comment at: lib/CodeGen/CGBuiltin.cpp:7673
 
-Value *CodeGenFunction::EmitX86CpuInit() {
+Value *CodeGenFunction::EmitX86CpuInit(CGBuilderTy &Builder) {
   llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy,
----------------
echristo wrote:
> Why do you need to pass in a Builder?
Because when I first wrote this, the EmitResolver function was a free function.  I guess I missed this dependency along the way, thanks for the catch!


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2324
+              llvm::Triple::x86_64) &&
+         "Only implemented for x86 targets");
+
----------------
echristo wrote:
> Can you get here via trying to compile code for another cpu?
At the moment, no.  I'm asserting to make sure that is the case, and to be a hint for George/et-al who are implementing this in the future for other processors.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:840-841
+
+  const auto *ND = cast<NamedDecl>(GD.getDecl());
+  UpdateMultiVersionNames(GD, ND);
+
----------------
rsmith wrote:
> 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?)
Unfortunately it is too late at that point.  The problem is you have:

TARGET_SSE MVFunc(); 
TARGET_DEF MVFunc(); // At this point, a mangling-conflict occurs.

void foo() { 
MVFunc(); // Only at THIS point does the IFunc get created, too late to rewrite the SSE variant's name.  
}

That said, I moved it to a more appropriate place.


================
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>())
----------------
rsmith wrote:
> 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.)
The actual case I encountered was that when emitting the global in emitMultiVersionFunctions, the EmitGlobalDefinition was immediately skipping it here because it was an inline function.

I believe the correct response is to just call EmitGlobalFunctionDefinition from the emitMultiVersionFunctions handler.  This will have to be modified when I support virtual functions or constructors, but this will make it work.

IMO, the cross-TU issue you come up with would only be an issue when the value isn't emitted due to it being inline/static/etc.  In this case, the failed linking is an acceptable consequence to the user improperly providing a multiversion variant.


================
Comment at: lib/Sema/SemaDecl.cpp:9720
+  if (CheckMultiVersionFunction(*this, NewFD, Redeclaration, OldDecl,
+                                MergeTypeWithPrevious, Previous))
+    return Redeclaration;
----------------
rsmith wrote:
> 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?
Looks like I'd just grabbed the wrong name for the parameter in CheckMultiVersionFunction.  Fixed!


================
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);
+}
----------------
rsmith wrote:
> What about uses in contexts where there is no target function type? For example, `+foo;`
Added as a test in Sema/attr-target-mv.c.


================
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; }
+};
----------------
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.  


https://reviews.llvm.org/D40819