Page MenuHomePhabricator

[AIX] Implement by-val caller arguments in a single register
ClosedPublic

Authored by cebowleratibm on Mar 9 2020, 10:19 AM.

Details

Summary

This is the first of a series of patches that adds caller support for by-value arguments that are passed in a single GPR.

There are 3 limitation cases:
-The by-value argument is larger than a single register.
-There are no remaining GPRs even though the by-value argument would otherwise fit in a single GPR.
-The by-value argument requires alignment greater than register width.

Future patches will be required to add support for these cases as well as for the callee handling (in LowerFormalArguments_AIX) that corresponds to this work.

Diff Detail

Event Timeline

cebowleratibm created this revision.Mar 9 2020, 10:19 AM
hubert.reinterpretcast retitled this revision from [AIX} Implement by-val caller arguments in a single register to [AIX] Implement by-val caller arguments in a single register.Mar 9 2020, 11:37 AM
cebowleratibm marked an inline comment as done.Mar 10 2020, 8:02 AM
cebowleratibm added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7112

To reviewers, I thought I had a bug that we may choose a PowerOf2Floor that's larger than a register. This will be a bug, however, for the time being, we're only processing ByValSize <= PtrByteSize.

sfertile added inline comments.Mar 11 2020, 8:42 AM
llvm/test/CodeGen/PowerPC/aix-cc-abi.ll
1351

This test is already ridiculously big, I suggest we add these changes into a new test file instead.

sfertile added inline comments.Mar 12 2020, 10:35 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7125

IIUC EXTLOAD is safe for the first load because it will be left justified in the register after the shift, but the later loads need the upper bits defined to zero because we are ORing them into those shifted bits.

cebowleratibm planned changes to this revision.Mar 16 2020, 10:02 AM
cebowleratibm marked 3 inline comments as done.
  1. Split byval cc tests into a separate file.
  2. Use ZEXTLOAD
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7125

Good catch. Using ZEXTLOAD improved the codegen to use rotate/mask inserts to assemble the byval in register. See the test updates on the next revision.

cebowleratibm marked an inline comment as done.

-Use ZEXTLOAD rather than EXTLOAD. This changed expected output because we're able to use rotate left word immediate with mask form instructions.
-Split byval cc tests into new test files.

cebowleratibm planned changes to this revision.Mar 17 2020, 7:43 AM
cebowleratibm marked an inline comment as done.
cebowleratibm added inline comments.
llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
1 ↗(On Diff #250593)

I missed the run commands. Need to add them and update review.

ZarkoCA added inline comments.Mar 17 2020, 7:50 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7139

I'm curious as to what happens with 0 sized parameter, will they need special handling like they do on the callee side or do they need to be skipped?

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
1 ↗(On Diff #250593)

Caught by the automatic testing bots, this test doesn't have a RUN step.

197 ↗(On Diff #250593)

looks like a git artifact here

Added the missing run steps to aix-cc-byval.ll

cebowleratibm planned changes to this revision.Mar 17 2020, 9:40 AM
cebowleratibm marked 4 inline comments as done.

-Add CC_AIX logic to pass over 0 size by-val arguments.
-Tidy the by val tests added.
-Some expected output changes required in aix-cc-byval.ll related to use of ZEXTLOAD now that I've added the run step.

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

We should skip 0 size by-val arguments in CC_AIX and assert non-zero by-val size here.

cebowleratibm marked an inline comment as done.

-Added skipping of 0 size by val args.
-Tidied up tests.

sfertile added inline comments.Mar 17 2020, 3:54 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7098

I don't think we need an assertion like this: CCValAssign is either a RegLoc or a MemLoc. by the class construction. Its not possible to create an object with both of these false. If the class design ever changes such that there are other options then we can add an assertion then.

7109

Could we do a larger load if the alignment is greater then the size? For example if we have a size of 7 and a type alignment of 1, but the argument is an auto local and the compiler increase the alignment to 8. I don't suggest making any such changes to this patch if it allowed, but something to consider once we have everything working.

7119

suggestion:
Move the load over --> Adjust the load offset

7131

Why the name PaddingInBits? Isn't this is the shift size to justify the bytes just loaded, where does padding come in?

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
38 ↗(On Diff #250884)

IDX isn't used again. You can use just the regex to match the label without having to store it in a file check variable. Ditto on the other filecheck variables that aren't used after capture.

125 ↗(On Diff #250884)

Can we drop the 'BYTE' part of REG2BYTE and REG1BYTE?

136 ↗(On Diff #250884)

LC2(2) --> LC{{[0-9]+}}(2)

156 ↗(On Diff #250884)

Same fix mentioned above with 'LC2(2)'.

llvm/test/CodeGen/PowerPC/aix64-cc-byval.ll
27 ↗(On Diff #250884)

Same suggestion about dropping 'BYTE' from the variables name.

36 ↗(On Diff #250884)

Same fix on the label here and elsewhere in the test : LC{{[0-9]+}}

cebowleratibm planned changes to this revision.Mar 17 2020, 5:35 PM
cebowleratibm marked 12 inline comments as done.
cebowleratibm added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7109

The size should be a multiple of the alignment. We can do a single load if we can prove PtrByteSize bytes are accessible from the argument address. The auto local case you mention is probably the one worth looking into but I agree it's beyond the scope of this patch to optimize this case.

7131

"Padding" was meant to indicate the number of zero extended bits from the load that need to be left shifted to get the loaded bits into the correct bit offset. I agree the term "Padding" is misleading. I'll rename to "NumSHLBits".

llvm/test/CodeGen/PowerPC/aix-cc-byval.ll
125 ↗(On Diff #250884)

I'll just use REG1, REG2 ... as per usual generic reg names. The reader will need to chase down the contents.

I found it useful to name the regs by content as I tried to keep everything straight in my head, but I don't mind the more condensed form.

cebowleratibm marked 2 inline comments as done.

Addressed Sean's comments:
-General test tidy up, remove an unnecessary assertion, tweaked a variable rename.

sfertile accepted this revision.Mar 18 2020, 6:10 AM

LGTM.

This revision is now accepted and ready to land.Mar 18 2020, 6:10 AM
This revision was automatically updated to reflect the committed changes.