Patch adds support for passing vector arguments to variadic functions. Arguments which are fixed shadow GPRs and stack space even when they are passed in vector registers, while arguments passed through ellipses are passed in(properly aligned GPRs if available and on the stack once all GPR arguments registers are consumed.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/PowerPC/PPCCCState.h | ||
---|---|---|
41 | Should this derive from PPCCCState? If not, I think PPCCCState should be renamed something to indicate not AIX because AIX is still a PPC calling convention. | |
68 | const | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
6301 | Can you define: typedef bool AIXCCAssignFn(unsigned ValNo, MVT ValVT, MVT LocVT, CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags, AIXCCState &State); and retype the AIXCCState functions accordingly? I think this would be more typesafe to avoid the need to static cast the state. | |
6705 | By my understanding, if we have a custom memloc for a vector arg then there must be either 2 or 4 custom reg locs that follow. 2 for the 32-bit "split" mem/reg case, or 2/4 for 32/64 bit shadow regs. I think it would be better to structure this with the explicit logic to expect the 2 or 4 custom regs that follow with assertions, rather than use a loop to make the caller/callee contract fully explicit. |
llvm/lib/Target/PowerPC/PPCCCState.h | ||
---|---|---|
41 | IIUC PPCCCState is exclusively used by the 32-bit ELF target. When we add support for fp128 on AIX we might need the same functionality for 32-bit AIX at which point we would probably inherit from it instead of the base class. I can rename PPCCCState to ELF32CCState (I need to double check it is indeed only used by the 32-bit target first) | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
6301 | Sure, but then I would have to duplicate CCState::AnalyzeFormalArguments and CCState::AnalyzeCallOperands functions in AIXCCState since the types for AIXCCAssignFn differs from the type of CCAssignFn. Is this acceptable? I picked the static cast in the assign function since we do something similar in the table-gen assign functions already, eg: CCIf<"!static_cast<PPCCCState *>(&State)->WasOriginalArgPPCF128(ValNo)",` |
- Converted loop to explicit handling of custom RegLocs
- Added const on member function.
llvm/lib/Target/PowerPC/PPCCCState.h | ||
---|---|---|
41 | I'll put the rename into a separate NFC patch. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6725 | maybe adding an assert for !IsPPC64 would be useful? |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6301 | You had to declare AIXCCState::AnalyzeFormalArguments and AIXCCState::AnalyzeCallOperands in this patch anyways. It was my observation that you could declare them with AIXCCAssignFn instead to enforce the type safety and avoid the static_cast. I can accept the static cast but I think the suggestion is safer for a small effort. | |
6719 | Suggestion: assert that VA.getValNo() matches across the set of custom mem/reg locs that we expect to see. | |
llvm/test/CodeGen/PowerPC/aix32-vector-vararg-callee-split.ll | ||
33 | it's subtle to see how this involves vector CC. I gather this is the IR we would see for a va_arg call with a vector int type. Maybe worth a comment? |
- Add asserts that the ValNo of custom RegLocs match the ValNo of the custom MemLoc.
- Assert that we are targeting 32-bit codegen when we have 4 custom RegLocs.
- Add comments to the tests to make it easier to determine what is being tested.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6301 | The definitions I add in AIXCCState forwards the assign function on to the base class implementation though. If I change the type of the assign function we have a type mismatch between the assign function type AIXCCState uses and the type of assign function the base class expects. Thats simple to solve, I can duplicate the code from the base class inline in AIXCCState. I am happy with either approach but wanted to make sure you understood getting the type safety in the assign function signature come with the cost of having to duplicate that code. | |
6719 | Good idea, added. | |
6725 | Good idea, added. |
LGTM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6301 | I am ok with either approach that you want to take. |
Looks like this class was created only to determine whether an argument is fixed? That said, to me this feels like the cleanest way to do this.