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@Z.resolver"(i32) comdat
+// WINDOWS: call i32 @"?foo_overload@@YAHH@Z.arch_sandybridge"
+// WINDOWS: call i32 @"?foo_overload@@YAHH@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
