Page MenuHomePhabricator

[NFC] Refactor TargetCallingConv so calling conventions can see which parameters are varargs
Needs ReviewPublic

Authored by comex on May 1 2019, 6:08 PM.

Details

Summary

Currently, the OutputArg struct has a flag IsFixed, where "fixed" means "not a variable argument". But the value of this flag is not provided to calling convention functions. As a result, 3 different targets (Mips, SystemZ, Hexagon) define their own "CCIfArgIsVarArg" or equivalent, of which 2 work by maintaining a separate std::vector of which arguments are variable, while the other uses a subclass of CCState. In addition, at least AArch64 avoids using CCState::AnalyzeCallOperands entirely, seemingly because of this issue; instead, it does its own loop over the arguments, choosing a calling convention for each argument based on IsFixed (as well as other checks that don't vary by argument; see AArch64TargetLowering::CCAssignFnForCall).

As far as I can tell, there's no good reason for things to be this way. OutputArg has another field Flags, of type ArgFlagsTy, which already tracks a wide variety of information about each argument, and which *is* passed to calling convention functions. This patch simply moves IsFixed to be a flag within ArgFlagsTy (renaming it IsVarArg for consistency). Then it adds non-target-specific CC interfaces CCIfArgIsVarArg and CCIfArgNotVarArg, which can now just check ArgFlags.isVarArg() rather than doing anything fancy, and removes the aforementioned target-specific definitions in favor of them.

Why the verbose names? Because there are already interfaces CCIfVarArg and CCIfNotVarArg, which test whether the *function* is declared as receiving variable arguments, as opposed to the current argument being variable. To avoid confusion, this patch also renames those to CCIfFuncIsVarArg and CCIfFuncNotVarArg.

One caveat is that ArgFlagsTy is also used for input arguments, which cannot be varargs, but that's no big deal since isVarArg() will just return false.

I also needed to refactor some code in the Mips backend: it was relying on the fact that InputArg and OutputArg happened to have the same type signature, even though the meaning of the parameters was different! Specifically, InputArg has a bool parameter Used, while OutputArg had a bool parameter IsFixed in the same place. With IsFixed removed, the code broke. I refactored it to use a callback instead of a template: a bit ugly, but so was the old version.

However, this patch doesn't fix all the hacks I noticed. MipsCCState.h and SystemZCallingConv.h both have comments claiming AnalyzeCallOperands "is not usable since we must provide a means of accessing" IsFixed, yet the replacement functions defined in those files tracked other things in addition to IsFixed. I've removed the CallOperandIsFixed vector from both, but I haven't investigated whether the other vectors can be removed too and the code refactored to use AnalyzeCallOperands, so instead I added TODO comments. I also haven't refactored the AArch64 code to use AnalyzeCallOperands.

Diff Detail

Event Timeline

comex created this revision.May 1 2019, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 6:08 PM

Looks like a nice tidy-up, just a few style issues.

lib/Target/AArch64/AArch64ISelLowering.cpp
3617

This can re-use the ArgFlags variable. We can probably drop the comment now, I think the name makes this obvious now.

lib/Target/Mips/MipsCCState.h
99

This comment is referring to code which has now been deleted, could it be re-written to just mention the current issues?

lib/Target/SystemZ/SystemZCallingConv.h
59–60

Another comment that should be re-worded to only refer to the current code.

comex updated this revision to Diff 199337.May 13 2019, 3:08 PM

Thanks for the feedback, updated.