If the varargs are not accessed by a function, we can inline the
function.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
"musttail" calls forward varargs arguments to the callee.
Otherwise, the transform looks okay.
Comment Actions
Ah thanks I was not sure about that! So in the test case, we should have to forward i32 42 to the musttail call, e.g. call void (i8*, ...) bitcast (void (i8*, i32)* @ext_method to void (i8*, ...)*)(i8* %this_adj.i, i32 42)? If that's the case, then it makes sense to keep the forwarding code in InlineFunction and I'll update D41336 accordingly.
Comment Actions
Thank you very much Eli for clarifying this! I've updated the patch to forward the passed varargs for musttail calls. I'll follow up with another patch that preserves all attributes.
Comment Actions
We should probably clarify LangRef for musttail to make it clear what transforms are legal. (Does the "thunk" attribute matter?)
| lib/Transforms/Utils/InlineFunction.cpp | ||
|---|---|---|
| 1516 ↗ | (On Diff #127523) | Does the comment from https://reviews.llvm.org/D22529#488888, about doing this check during the inline cost scan, still apply here? |
Comment Actions
I think for this optimization, what matters is the fact that we need to forward the varargs from the call site to the musttail function. Although I am not sure if the following statement from the LangRef is very clear about the forwarding behaviour. Is it worth clarifying that?
Both markers imply that the callee does not access allocas or varargs from the caller.
The callee must be varargs iff the caller is varargs. Bitcasting a non-varargs function to the appropriate varargs type is legal so long as the non-varargs prefixes obey the other rules.
| lib/Transforms/Utils/InlineFunction.cpp | ||
|---|---|---|
| 1516 ↗ | (On Diff #127523) | Ah thanks, I was not aware of D22529. I can move the check to the inline cost scan. The only potential problem could be that the inline cost check can be disabled AFAIK and then this would affect legality. |
Comment Actions
"The callee must be varargs iff the caller is varargs" sort-of implies the forwarding, but probably worth stating more explicitly.
Comment Actions
Moved legality check to InlineCost.cpp
It seems to fit nicely into InlineCost.cpp. The only problem is that for example the PartialInliner uses InlineFunction() without using InlineCost, so it misses all the legality checks from there. But there are already plenty of cases where InlineCost checks for legality, so this should be addressed in the PartialInliner IMO.
| lib/Transforms/Utils/InlineFunction.cpp | ||
|---|---|---|
| 1838–1843 ↗ | (On Diff #127734) | 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@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. |
| test/Transforms/Inline/inline-musttail-varargs.ll | ||
| 4–5 ↗ | (On Diff #127734) |
:) |
| test/Transforms/Inline/inline-varargs.ll | ||
| 18 ↗ | (On Diff #127734) | 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. |
| lib/Transforms/Utils/InlineFunction.cpp | ||
|---|---|---|
| 1838–1843 ↗ | (On Diff #127734) | Yep thanks. I was planning to fix the attributes & co, but I'll do it before this patch lands. |
| test/Transforms/Inline/inline-varargs.ll | ||
| 18 ↗ | (On Diff #127734) | I'll check! |
| lib/Transforms/Utils/InlineFunction.cpp | ||
|---|---|---|
| 1838–1843 ↗ | (On Diff #127734) | |
Comment Actions
Post holiday ping.
I've removed -instcombine from the test, as it is not needed.
| test/Transforms/Inline/inline-varargs.ll | ||
|---|---|---|
| 18 ↗ | (On Diff #127734) | |
Comment Actions
Hi, I am trying to build Android with clang 7.0.
This change generates wrong inlined sprintf because Android bionic
used fortified version wrapper with attribute((always_inline)).
Please try this simplified sprintf.c code.
typedef __builtin_va_list va_list;
void do_sprintf1(char*, const char*, va_list);
void do_sprintf2(char*, const char*, va_list);
static inline __attribute__((__always_inline__))
void mysprintf1(char *dest, const char* format, ...)
{
va_list va;
__builtin_va_start(va, format);
do_sprintf1(dest, format, va);
__builtin_va_end(va);
}
static inline
void mysprintf2(char *dest, const char* format, ...)
{
va_list va;
__builtin_va_start(va, format);
do_sprintf2(dest, format, va);
__builtin_va_end(va);
}
char* test_sprintf(char *dest, const char *format)
{
mysprintf1(dest, format, 1);
mysprintf2(dest, format, 2);
return dest;
}When compiled with gcc, gcc rejects that attribute:
$ gcc -S sprintf.c sprintf.c: In function 'mysprintf1': sprintf.c:6:6: error: function 'mysprintf1' can never be inlined because it uses variable argument lists void mysprintf1(char *dest, const char* format, ...) ^~~~~~~~~~
When compiled with older clang compiler, the always_inline attribute was ignored.
Neither mysprintf1 or mysprintf2 was inlined.
With this change, mysprintf1 is inlined incorrectly due to the always_inline attribute.
Could someone suggest a fix or revert this change?
Thanks.
Comment Actions
Yes, thanks for pointing that out Eli. I can provide a fix tomorrow. We should probably try to get this back-ported to 6.0.