This is an archive of the discontinued LLVM Phabricator instance.

Inlining of empty/small variadic functions.
AbandonedPublic

Authored by Sunil_Srivastava on Jul 19 2016, 1:47 PM.

Details

Summary

This is a fix for PR27796, inlining of empty/small variadic

While the general problem of inlining variadic functions is difficult, it is quite easy to inline variadic functions if they do not use va_start, because in those cases we can simply ignore arguments passed against '...'. That use pattern, though seemingly wasteful, is actually quite useful for writing functions for trace messages, with bodies that are removed via conditional compilation in production builds. This is done with the expectation that the compiler will inline these essentially empty functions and optimize out entire tracing overhead.

That is the purpose of this proposed change.

The determination of whether a the called function if using va_start is done at the time of inlining by scanning the code of the called function. However, in order to avoid the cost of this extra scan, we limit this scan to 'small' function; the definition of 'small' being that it must have a single block of no more that 20 instructions. Larger callees are rejected without full scan. That is sufficient for our use cases of mostly empty functions. The choice of 20 for number of instructions is somewhat arbitrary.

Diff Detail

Event Timeline

Sunil_Srivastava retitled this revision from to Inlining of empty/small variadic functions..
Sunil_Srivastava updated this object.
Sunil_Srivastava added a subscriber: llvm-commits.
chandlerc requested changes to this revision.Jul 19 2016, 1:59 PM
chandlerc edited edge metadata.

I think we should do this during the inline cost scan, which has a natural thresholding effect. That will also allow us to inline variadic functions where there is a use of 'va_start', but only behind a condition that is known to be false at the callsite we're inlining through.

There are a few other places where we bail out of the cost analysis if we see a construct that simply can't be handled, and it feels like this would fit well there.

This revision now requires changes to proceed.Jul 19 2016, 1:59 PM
Sunil_Srivastava edited edge metadata.

This is an update to inlining-of-variadic-functions change along the lines of suggestion by Chandler.

The determination to attempt or not is being made in InlineCost.cpp.

Presence of a va_start or a MustTail call in a variadic callee disqualifies it from being inlined. Other than that they can be inlined.

Changes in InlineFunction.cpp are merely for:

  • to remove prohibition on inlining of variadic functions
  • assertion checks that a variadic function having a va_start or MustTail do not slip through the cost analysis.

I do have one concern: Should I worry about plain 'tail' calls as well, same as 'musttail' calls ? Looking for opinions.

This is an update to inlining-of-variadic-functions change along the lines of suggestion by Chandler.

The determination to attempt or not is being made in InlineCost.cpp.

Presence of a va_start or a MustTail call in a variadic callee disqualifies it from being inlined. Other than that they can be inlined.

What's the motivation to inline cases where a variadic function calls some other variadic function but not with 'musttail'? I understand that such a call can't usefully call 'va_start', but such a call seems unlikely to matter regardless? If this is important to handle for some reason, I think it would be important to understand why. If it isn't important, it would be much simpler to just match the old behavior but inside inline cost where we can ignore uses along dead code paths.

I am a bit worried about C code with direct, trivial forwarding from one function to another without anything *explicitly* marking the inner call such that it is musttail, and just relying on a tail call occurring....

Some minor nits below, not sure they will be relevant depending on the answer to the issues above.

lib/Transforms/Utils/InlineFunction.cpp
1700

nit: wrong naming convention for variable...

1706–1709

nit: formatting is weird here.

What's the motivation to inline cases where a variadic function calls some other variadic function but not with 'musttail'?

Hmm. I don't think I am looking for variadic function calling other variadic function; with or without 'musttail'.
Note that CallAnalyzer::FIsVarArg indicates whether CallAnalyzer::F is variadic, not whether any callee of CallAnalyzer::F is variadic. So if the call being considered for inlining is of A calling B, then the change being proposed only considers whether B is variadic, and if it is, then it also considers whether any call inside B is musttail.

Could you give an example of what you are concerned about here ?

I understand that such a call can't usefully call 'va_start', but such a call seems unlikely to matter regardless? If this is important to handle for some reason, I think it would be important to understand why. If it isn't important, it would be much simpler to just match the old behavior but inside inline cost where we can ignore uses along dead code paths.

We can address this after I understand exactly what your concern is.

I am a bit worried about C code with direct, trivial forwarding from one function to another without anything *explicitly* marking the inner call such that it is musttail, and just relying on a tail call occurring....

That one I am also concerned about, though I don't know why that would be a worry in C only. Any suggestions about it? Would setting such cloned calls to NoTail work ?

In the worst case we could just disqualify variadic callees having any calls irrespective of any tail-criteria. At least our use cases of interest are situation where the variadic callee is empty.

A friendly ping please

If it's taking more than a couple of weeks to get a response from a reviewer, you should email them directly; some people filter mail to llvm-commits into a separate mailbox, and don't notice pings on the mailing list.

hfinkel added inline comments.
lib/Analysis/InlineCost.cpp
903 ↗(On Diff #72927)

Really? Does the LangRef guarantee this?

lib/Analysis/InlineCost.cpp
903 ↗(On Diff #72927)

First, I must admit that I do not fully know where musttail calls are used. I believe they are used for thunks.

LangRef does not even have the concept of musttail, so I can not answer your question precisely.

I am trying to be conservative and try to prevent C from accessing A-to-B unnamed arguments via some builtins or some other way in a situation like this:

void C(int x, ...) // actual user code.
{

va_start(ap, x); 
assert(va_arg(ap, int) == 5);

}
void B(int a, ...) // a thunk meant to jump to C
{

.. do something. Maybe a+=4 ..
C(a, ....); // musttail call somehow. Essentially a jump to C

}
void A(int x){

B(a, 5);

}

Now if we inline B into A, we will throw away 5 before C is even entered. Therefore I want to prevent B-into-A inlining.

This revision now requires changes to proceed.Jun 19 2017, 2:32 PM
Sunil_Srivastava marked an inline comment as done.

Took care of the 'nits' and updated to the current version of llvm.

No logical changes. Still waiting for response on logical points.

Sunil_Srivastava abandoned this revision.Feb 21 2018, 11:56 AM