Page MenuHomePhabricator

Copy Function's calling convention by default when creating a new Call
Needs ReviewPublic

Authored by serge-sans-paille on Nov 20 2017, 6:51 AM.

Details

Summary

Altough calling a function whith mismatching Calling Conventions (CC) is defined in the LangRef (as UB), it is a common source of code duplication to set it based on the called function (when it's known), or to forget setting it (which leads to the call being replaced by an undef later in the optimization pipeline).

This commit enforces a default behavior when creating a new CallInst or InvokeInst, as discussed in this thread:

http://lists.llvm.org/pipermail/llvm-dev/2017-November/119121.html

Diff Detail

Event Timeline

rnk edited edge metadata.Nov 20 2017, 1:40 PM

This wasn't what I had in mind. When a frontend (or any other IR generator) creates a call, it should have a source of type information telling it the function prototype and the calling convention (in clang, the AST). It should also apply any other wacky calling convention flags like -mregparm and other things. In the special case where the frontend is emitting a call to a function defined in the current TU, we should have a check that the conventions match. I was imagining that you'd make IRBuilder implement that assert, and then add an alternative CreateCallWithConvention entry point or something like that to allow the frontend to override the check.

In D40249#930728, @rnk wrote:

This wasn't what I had in mind. When a frontend (or any other IR generator) creates a call, it should have a source of type information telling it the function prototype and the calling convention (in clang, the AST). It should also apply any other wacky calling convention flags like -mregparm and other things.

I totally agree. This means extending the current `Create with a CallingConv::ID` parameter to be able to express that in the builder. Note that my implementation goes in the same spirit (if no information, use the function's CallingConvention)

In the special case where the frontend is emitting a call to a function defined in the current TU, we should have a check that the conventions match. I was imagining that you'd make IRBuilder implement that assert, and then add an alternative CreateCallWithConvention entry point or something like that to allow the frontend to override the check.

I'm not sure that's the best approach. With the approach described above, either the user does not specify the callingconv and it's always consistent (if the function is statically known, that is), or it forces it and we have to trust him/her. Asserting while the LangRef allows mismatching CallingConv seems strange to me. Helping the user to avoid forgeting to set the calling conv looks safer.

@rnk: I've implemented the extra CallingConvention argument in the various create statements, but I'm very unhappy with the result: it requires a lot of extra create overload, the number of optional arguments grows and the overall gain within the actual codebase is small. I honestly think my current proposal, even if modest, is already a good step forward.

As an argument, consider the fact that this patch removes 41 lines of mostly redundant code.

I'd feel better about this if there was some other helper we used to set inreg and other ABI attributes on these calls.

restoring previous diff, I accidentally uploaded an incorrect one on that review :-/