This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Implement by-val caller arguments in multiple registers
ClosedPublic

Authored by cebowleratibm on Mar 18 2020, 12:29 PM.

Details

Summary

Ths patch builds upon commit rGc21866476e14, which added support for by-value arguments in a single register. With this patch the by-value argument support on AIX is extended to by-value arguments that will fit within available GPRs.

I've moved the 5-8 byte tests, which were aix64 specific into aix-cc-byval.ll because they now work as multi GPR tests in 32-bit. I've added a 64-byte by-value argument as an aix64 only test. When the caller by-value support is complete, the aix-cc-byval-limitation tests will be deleted and the aix64-cc-byval.ll tests should be folded into aix-cc-byval.ll.

Remaining caller by-value argument work:
-by-val args, which require alignment greater than PtrByteSize.
-by-val args, which pass either partially or completely in stack memory.

Diff Detail

Event Timeline

cebowleratibm created this revision.Mar 18 2020, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 12:29 PM
sfertile added inline comments.Mar 19 2020, 9:24 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7151

If all we need PeekVA for is its ValNo lets do unsigned ValNo = ArgLocs[I].getValNo()

7166

We do this same operation twice in this function, and the same pattern appears several times elsewhere in the file. I suggest putting it in a well named helper function. Only bother using for the 2 uses in this function for this patch, then we can convert some of the other uses in NFC patches.

7167

If we are setting the no-wrap flags maybe we should set both.

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
208

I don't see a lot of utility in testing each of 1 through 8 bytes with nearly exactly the same test. The are each so similar that we end up with a long verbose test with not a lot of practical coverage. Instead how about we add

  1. one of 5,6,7,8 bytes but with other arguments before/after the by val.
  2. A test with a couple byvals mixed with other arguments.
  3. One test with a 'large' by val like { [32 x i8] }.

Also use a struct type that isn't just containing an array. Ex if you choose 8 bytes as the size for the test in case 1 use: { i16, i16, i16, i16} as the struct type.

cebowleratibm marked 2 inline comments as done.Mar 20 2020, 8:07 AM
cebowleratibm added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7167

Are you referring to setNoSignedWrap?

Digging a bit more, I think we can use SelectionDAG::getObjectPtrOffset

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
208

1 and 2. I agree.

  1. I already have 31, 32 and 64 byte tests.

I'll change the struct layout in some of the tests.

cebowleratibm marked 5 inline comments as done.Mar 20 2020, 1:25 PM

I still have to update the tests as per Sean's suggestion but I'll post the updated code for now.

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

Fair point. I've opted to use a lambda to common this up.

7167

getObjectPtrOffset doesn't setNoSignedWrap but if that's suboptimal I think that's another patch entirely.

cebowleratibm marked an inline comment as done.

Updated code to address comments.

Test updates still required.

I split out some of the suggested test improvements to https://reviews.llvm.org/D76875

The patch has been rebased on https://reviews.llvm.org/D76401 ([PowerPC][AIX] ByVal formal argument support: single register.)

Note I've moved the 5-8 byte byval caller tests to aix-cc-byval.ll and left the 5-8 byte byval callee tests in aix64-cc-byval.ll.

I fixed a bug on the byval AllocateStack call in CC_AIX, which needs to reserve ByValSize bytes, rounded up to a multiple of PtrByteSize.

cebowleratibm marked 2 inline comments as done.Mar 26 2020, 12:03 PM
cebowleratibm added inline comments.
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
208
  1. is addressed in https://reviews.llvm.org/D76875
  2. Was already addressed in https://reviews.llvm.org/D76401 (thanks Sean)
  3. This patch has tests for 31 and 32 byte byvals.

Forgot the 64 byte byval test.

Clang format.

sfertile added inline comments.Mar 31 2020, 12:20 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7157

Real minor nit: Pass the Offset in as an argument.

Tweaked the lambda capture on GetLoad.

I rewrote some of the changes in preparation for the next commit I have planned for caller by-value arguments in stack memory. The changes in this diff should be NFC from the last revision.

cebowleratibm marked an inline comment as done.Mar 31 2020, 4:37 PM

I suggest adding a test along the lines of:

struct F {
  float x, y, z;
};

int callee(struct F);

int caller(void) {
  struct F s = { 0.0f, 0.0f, 0.0f };
  return callee(s);
}
  1. To show that homogeneous float aggregates are still passed in gprs.
  2. So that not all the tests load a global value for the argument.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7158

Are 'I != E' and ArgLocs[I].getValNo() == ValNo not both implied by LoadOffset + PtrByteSize <= ByValSize? They are probably better as assertions inside the loop body.

cebowleratibm marked an inline comment as done.Apr 1 2020, 6:36 PM

Added a test for the homogeneous float stuct Sean suggested.

Also made an NFCI change that moved part of a loop condition to an assertion of the loop body.

ZarkoCA added inline comments.Apr 2 2020, 6:50 AM
llvm/test/CodeGen/PowerPC/aix64-cc-byval.ll
53

I don't see a 64BIT check prefix in this file. I think these should be CHECK instead.

cebowleratibm marked 2 inline comments as done.Apr 2 2020, 9:47 AM
cebowleratibm added inline comments.
llvm/test/CodeGen/PowerPC/aix64-cc-byval.ll
53

Good catch. I'll also add the missing assembly expected output.

cebowleratibm marked an inline comment as done.

Fixed expected output for 64-byte byval arg test.

sfertile accepted this revision.Apr 2 2020, 1:42 PM

LGTM.

This revision is now accepted and ready to land.Apr 2 2020, 1:42 PM
This revision was automatically updated to reflect the committed changes.