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

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 20 10:10:10 PST 2017


rnk 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);
----------------
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.


================
Comment at: test/Transforms/Inline/inline-musttail-varargs.ll:4-5
-
-; We can't inline this thunk yet, but one day we will be able to.  And when we
-; do, this test case will be ready.
-
----------------
> ; We can't inline this thunk yet, but one day we will be able to.  And when we
> ; do, this test case will be ready.

:)



================
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)
+
----------------
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.


https://reviews.llvm.org/D41335





More information about the llvm-commits mailing list