Page MenuHomePhabricator

[InlineFunction] Inline vararg functions that do not access varargs.
ClosedPublic

Authored by fhahn on Dec 17 2017, 4:25 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Dec 17 2017, 4:25 PM

"musttail" calls forward varargs arguments to the callee.

Otherwise, the transform looks okay.

"musttail" calls forward varargs arguments to the callee.

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.

Yes, that's right.

fhahn updated this revision to Diff 127523.Dec 19 2017, 6:58 AM

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.

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?

fhahn added a comment.Dec 19 2017, 1:38 PM

We should probably clarify LangRef for musttail to make it clear what transforms are legal. (Does the "thunk" attribute matter?)

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.

"The callee must be varargs iff the caller is varargs" sort-of implies the forwarding, but probably worth stating more explicitly.

fhahn updated this revision to Diff 127734.Dec 20 2017, 8:21 AM

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.

rnk added inline comments.Dec 20 2017, 10:10 AM
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)

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

:)

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.

fhahn added inline comments.Dec 20 2017, 10:42 AM
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!

fhahn updated this revision to Diff 128086.Dec 23 2017, 10:13 AM

Add FIXME

fhahn marked an inline comment as done.Dec 23 2017, 10:16 AM
fhahn added inline comments.
lib/Transforms/Utils/InlineFunction.cpp
1838–1843 ↗(On Diff #127734)

I've added D41555 and D41556 to forward attributes & calling conventions. I could also put everything in this patch, but I thought this way it would be easier to review.

fhahn added a comment.Dec 29 2017, 1:21 PM

I think the issues raised by @rnk should be addressed in D41555 and D41556 . Those 2 patches are based on this one, and I was planning to commit them straight after committing this one.

test/Transforms/Inline/inline-varargs.ll
18 ↗(On Diff #127734)

I've opened D41633, which should address the issue.

fhahn updated this revision to Diff 128756.Jan 5 2018, 8:55 AM

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)

If the definition of ext_method is visible, instcombine can remove the cast (rL321706). If the definition is not visible, then removing the cast might not be save, after discussion in D41633.

LGTM, but please don't forget to update LangRef.

efriedma accepted this revision.Jan 5 2018, 12:02 PM
This revision is now accepted and ready to land.Jan 5 2018, 12:02 PM
This revision was automatically updated to reflect the committed changes.
chh added a subscriber: chh.Jan 25 2018, 12:32 PM
chh added a comment.Jan 25 2018, 12:40 PM

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.

Looks like we forgot to update llvm::isInlineViable(). Should be simple to fix.

fhahn added a comment.Jan 25 2018, 1:00 PM

Looks like we forgot to update llvm::isInlineViable(). Should be simple to fix.

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.

fhahn added a comment.Jan 25 2018, 1:29 PM

I've put up a patch D42556