Page MenuHomePhabricator

[PowerPC][AIX] ByVal formal argument support: multiple registers
ClosedPublic

Authored by sfertile on Mar 26 2020, 8:44 PM.

Details

Summary

Extends the ByVal formal argument handling for arguments that consume several registers.

Diff Detail

Event Timeline

sfertile created this revision.Mar 26 2020, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 8:44 PM

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

However, it seems like that will be the next patch that will be done. So it may be useful to save that test for it instead of having it here only to show the error.

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

"Passing structs split between registers..."?

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

This is a really good struct for a test.

cebowleratibm added inline comments.Mar 27 2020, 9:50 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7092–7093

"MemLocs already be handled"? :)

7110

Value capture on RegClass and LocVT? Ref capture works too but isn't necessary, though it does keep the captures simple. I raise this as a question because I'm not sure what the guidance should be for lambda captures.

7132

I would tend to write this as:

unsigned Offset = 0;
HandleRegLoc(VA, Offset);
Offset != PtrByteSize;
for(; Offset != StackSize; Offset +=PtrByteSize) {
  assert(ArgLocs[I].getValNo() == VA.getValNo() ...);
  HandleRegLoc(ArgLocs[I++], Offset);
}

This way we're effectively asserting that we initialize the expected number of bytes. "I != End" should be part of an assertion.

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

As before, readers may find the dead stores and failure to elide the store/load for the return odd. I suggest a brief comment as before.

896

same comment on dead stores / failure to elide store/load.

cebowleratibm added inline comments.Mar 27 2020, 9:51 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7132

Offset != PtrByteSize; => Offset += PtrByteSize;

sfertile updated this revision to Diff 253609.Mar 30 2020, 9:09 AM
  • Fixed wording on a fatal error.
  • Changed the args and captures on the lambda.
  • Restructured the loop as suggested.
  • Added comment about the dead-stores in the lit test.

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

This is a really good suggestion. Going to add this next.

A small suggestion would be to add a test to further show that the GPRs are "burned" properly with with floats in the byval handling. Eg. using a struct like `%struct.S31 = type <{ float, i32, i64, double, i32, double }>' and seeing the error that structs split regs and the stack are not handled.

This is a really good suggestion. Going to add this next.

I forgot that Chris already did this in D76875: see the @test_byval_2Byte.

sfertile marked 7 inline comments as done.Mar 31 2020, 7:10 AM
sfertile updated this revision to Diff 253954.Mar 31 2020, 12:17 PM

Add a test that checks for a fatal error when a ByVal argument is split across registers and stack.

cebowleratibm added inline comments.Apr 1 2020, 7:22 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7135

The first ArgLoc could be a mem loc, in which case the error is a bit odd. I think it's sufficient to use "Passing ByVals on the stack is not yet implemented." Change or leave it at your discretion.

7139

I think this assertion is redundant to the assertion that will occur on ArgLocs[I] anyways.

sfertile marked 2 inline comments as done.Apr 2 2020, 7:48 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7135

If you mean the first ArgLoc for the ByVal argument is a MemLoc then that is handled elsewhere, hence why this diagnostic is specific to splitting between registers and stack.

7139

This should have come before any of the reads of ArgLocs[I], I'll move it.

sfertile updated this revision to Diff 255492.Apr 6 2020, 2:23 PM
sfertile marked an inline comment as done.
  • rebase to pick up landed dependencies.
  • Moved assertion that loop index was still valid to top of loop.
  • added a test that exercise fatal error for args split between registers and stack.

I had intended to move the 5-8 byte byval callee cases out of aix64-cc-byval.ll and put them here. I don't want aix64-cc-byval.ll to exist when we're done. If the 5-8 byte tests aren't useful for the callee I'd rather delete them. They were of interest to the caller handling to make sure that we implement the left justification correctly.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7096–7099

We currently error on byval alignment > PtrByteSize. I'd be inclined to assert it here as well to make it really hard to emit invalid code.

(Noting that we need to decide how to handle 8 byte aligned structs in 32-bit mode)

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

I'm ok with the test as written, though I would have been inclined to write the test like this:

; ASM32-DAG:   stw 3, 24(1)
; ASM32-DAG:   stw 4, 28(1)
; ASM32-DAG:   stw 5, 32(1)
; ASM32-DAG:   stw 6, 36(1)
; ASM32-DAG:   stw 7, 40(1)
; ASM32-DAG:   stw 8, 44(1)
; ASM32-DAG:   stw 9, 48(1)
; ASM32-DAG:   stw 10, 52(1)
; ASM32-DAG:   lbz 3, 45(1)
; ASM32-NEXT:  blr

Understanding that this permits the lbz to move above the stw 3. It's easier to read, accepts more valid outputs at the expense of accepting some invalid outputs. It's been my preferred approach to these tests so far.

cebowleratibm accepted this revision.Apr 7 2020, 12:15 PM

LGTM.

My comments are only suggestions/discussion and can be addressed as you see fit on the commit.

This revision is now accepted and ready to land.Apr 7 2020, 12:15 PM
sfertile marked 2 inline comments as done.Apr 8 2020, 8:38 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7096–7099

Good point. I was expecting CC_AIX to handle it by either burning registers, or adjusting the stack offset to a properly aligned address and so it should be a nop in the formal argument handling. I added a fatal error so that I can remove it and add lit testing after we remove the limitation in CC_AIX.

This revision was automatically updated to reflect the committed changes.
sfertile marked an inline comment as done.