Extends the ByVal formal argument handling for arguments that consume several registers.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
7132 | Offset != PtrByteSize; => Offset += PtrByteSize; |
- 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.
Add a test that checks for a fatal error when a ByVal argument is split across registers and stack.
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. |
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. |
- 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. |
LGTM.
My comments are only suggestions/discussion and can be addressed as you see fit on the commit.
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. |
"MemLocs already be handled"? :)