[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2391-2393
+  llvm::SmallVector<llvm::Value*, 10> Args;
+  llvm::for_each(Resolver->args(),
+                 [&](llvm::Argument &Arg) { Args.push_back(&Arg); });
----------------
erichkeane wrote:
> rnk wrote:
> > Surely this would be simpler as a range for or `Args{Resolver->arg_begin() ...};`
> Surely... Unfortunately the types don't match.  Resolver Args are an array of Arguments (so the iterator is a Arg*).
> 
> HOWEVER, "Args" here needs to be an array ref to Value*, so the iterator would have to be a Value** (not a Arg*/Value*).  The point of this small vector is to create an array of _pointers_ from the array of Arguments.
Right. :)


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2395
+
+  llvm::CallInst *Result = Builder.CreateCall(FuncToReturn, Args);
+
----------------
erichkeane wrote:
> erichkeane wrote:
> > rnk wrote:
> > > This approach is... not going to work in the general case. Consider, for example, varargs. Then consider 32-bit x86 inalloca, which is quite widespread. Any non-trivially copyable object is not going to be passable through such a thunk. You might consider looking at CodeGenFunction::EmitMustTailThunk instead.
> > Oh dear... 
> > I'm unfamiliar with EmitMustTailThunk, is it self explanatory?  Any chance you can expound?  Should I call/use that function, or copy out of it?
> I looked through that function, and it seems to do very similar things to this...
> 
> It FIRST does the small-vector conversion to a value* array like I do.
> 
> Second, it 'adjusts' the 'this' parameter.  This doesn't seem like it needs to happen, because the type isn't changing, right?  my 'this' pointer is still pass-on-able.
> 
> Third, it creates the call, then marks it TCK_MustTail.
> 
> Forth, it sets the attributes/calling convention of the 'call' object, but I'd expect that to come from the llvm::Function object in the 'CreateCall', right?  I can add them after the fact I guess, if we see value.*(see below)
> 
> Finally, it does the CreateRetVoid/CreateRet like this function does below. 
> 
> *I DO have a test that claims that my IR is broken if I set the call attributes.  Curiously only 1 of my tests, but the error is:
> "cannot guarantee tail call due to mismatched ABI impacting function attributes"
> 
> The only difference in attributes is target-features and target-cpu though, so I don't know what causes this.
Right, the this-adjustment is definitely unnecessary for this kind of CPU dispatch thunk.

The important thing is that marking the call as `musttail` ensures that you get perfect forwarding of all arguments, even varargs and inalloca. Maybe it's enough to just mark the call you are creating that way. I think the code you are generating obeys all the musttail invariants, so that might be best.

The "cannot guarantee tail call due to mismatched ABI impacting function attributes" verifier check is only supposed to fire when parameter attributes like `byval` and `inreg` differ between the call site and the dispatcher function. It shouldn't care about target-cpu or target-features.


================
Comment at: test/CodeGenCXX/attr-target-mv-overloads.cpp:74
+// WINDOWS: define dso_local i32 @"?foo_overload@@YAHH at Z.resolver"(i32) comdat
+// WINDOWS: call i32 @"?foo_overload@@YAHH at Z.arch_sandybridge"
+// WINDOWS: call i32 @"?foo_overload@@YAHH at Z.arch_ivybridge"
----------------
erichkeane wrote:
> rnk wrote:
> > Does dumpbin demangle these names correctly? Just curious.
> I don't know, I'd suspect not.  However, demangler.com (not sure what it uses?) seems to demangle them ignoring everything after the '.'.
Neat.


Repository:
  rC Clang

https://reviews.llvm.org/D53586