This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Handle variadic vector formal arguments.
ClosedPublic

Authored by sfertile on Feb 25 2021, 9:32 AM.

Details

Summary

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.

Diff Detail

Event Timeline

sfertile created this revision.Feb 25 2021, 9:32 AM
sfertile requested review of this revision.Feb 25 2021, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 9:32 AM
ZarkoCA added inline comments.Feb 26 2021, 1:20 PM
llvm/lib/Target/PowerPC/PPCCCState.h
41

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.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6266
6269
6490–6491
6504
6529
6545
6701–6702
sfertile updated this revision to Diff 326966.Feb 28 2021, 7:52 AM

Fixed a number of formatting and spelling mistakes.

cebowleratibm added inline comments.Mar 1 2021, 2:14 PM
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.

6708

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.

sfertile added inline comments.Mar 2 2021, 7:12 AM
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)",`
sfertile updated this revision to Diff 327466.Mar 2 2021, 8:31 AM
  • Converted loop to explicit handling of custom RegLocs
  • Added const on member function.
sfertile marked 2 inline comments as done and an inline comment as not done.Mar 2 2021, 8:33 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCCCState.h
41

I'll put the rename into a separate NFC patch.

ZarkoCA added inline comments.Mar 2 2021, 9:24 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6728

maybe adding an assert for !IsPPC64 would be useful?

cebowleratibm added inline comments.Mar 2 2021, 2:57 PM
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.

6722

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?

sfertile updated this revision to Diff 327794.Mar 3 2021, 7:32 AM
sfertile marked an inline comment as not done.
  • 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.
sfertile marked 2 inline comments as done and an inline comment as not done.Mar 3 2021, 7:42 AM
sfertile added inline comments.
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.

6722

Good idea, added.

6728

Good idea, added.

sfertile updated this revision to Diff 327951.Mar 3 2021, 3:52 PM
sfertile marked 2 inline comments as done.

Fix formating on assert.

cebowleratibm accepted this revision.Mar 3 2021, 5:23 PM

LGTM

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6301

I am ok with either approach that you want to take.

This revision is now accepted and ready to land.Mar 3 2021, 5:23 PM
sfertile retitled this revision from PowerPC][AIX] Handle variadic vector formal arguments. to [PowerPC][AIX] Handle variadic vector formal arguments..Mar 4 2021, 6:34 AM
This revision was automatically updated to reflect the committed changes.