[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:2395
+
+  llvm::CallInst *Result = Builder.CreateCall(FuncToReturn, Args);
+
----------------
rnk wrote:
> 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.
It's probably worth adding a test that uses inalloca, since that's why we're doing this nonsense. It would look like:
```
struct Foo {
  Foo();
  Foo(const Foo &o);
  ~Foo();
  int x;
};
int __attribute__((target("default"))) bar(Foo o) { return o.x; }
int __attribute__((target("sse4.2"))) bar(Foo o) { return o.x + 1; }
int __attribute__((target("arch=ivybridge"))) bar(Foo o) { return o.x + 2; }
```

Targetting i686-windows-msvc.


https://reviews.llvm.org/D53586