[PATCH] D41335: [InlineFunction] Inline vararg functions that do not access varargs.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1838-1843
+          SmallVector<Value *, 6> Params(CI->arg_operands());
+          Params.append(VarArgsToForward.begin(), VarArgsToForward.end());
+          CallInst *Call =
+              CallInst::Create(CI->getCalledFunction() ? CI->getCalledFunction()
+                                                       : CI->getCalledValue(),
+                               Params, "", CI);
----------------
rnk wrote:
> I think you're forgetting to transfer argument attributes. Consider this C++ test case:
> ```
> struct ByVal { char x; short y; };
> struct Foo {
>   virtual int foo(struct ByVal o) { return o.x + o.y; }
> };
> int main() {
>   ByVal o = {3, 5};
>   auto mp = &Foo::foo;
>   return (Foo().*mp)(o);
> }
> ```
> 
> Compile it with `clang -cc1 t.cpp  -emit-llvm -triple i686-windows-msvc` to get LLVM IR, and there will be a thunk called `??_9Foo@@$BA at AE` that we should be able to inline after your change. However, it's important that we retain the `byval` attribute on the `o` argument, or we will have mismatched calling conventions.
> 
> Argument forwarding is actually quite involved. Take a look at ArgumentPromotion and DeadArgElim for examples of how to do it as correctly as we know how to.
Yep thanks. I was planning to fix the attributes  & co, but I'll do it before this patch lands.


================
Comment at: test/Transforms/Inline/inline-varargs.ll:18
+; CHECK-LABEL: define void @thunk_caller(i8* %p)
+; CHECK: call void (i8*, ...) bitcast (void (i8*, i32)* @ext_method to void (i8*, ...)*)(i8* %this_adj.i, i32 42)
+
----------------
rnk wrote:
> I was hoping instcombine would simplify this to `call void @ext_method(i8* %this_adj.i, i32 42)`. I hope there's no reason we can't and that instcombine just hasn't been taught how to do it yet, but let's leave a FIXME comment here for it.
I'll check!


https://reviews.llvm.org/D41335