[RISCV][GISel] Reverse the operands the buildStore created in legalizeVAStart. #73989
Conversation
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesWe need to store the frame index to the location pointed to by the VASTART, not the other way around. Apparently we're missing tests for G_VASTART so no tests changed. Full diff: https://github.com/llvm/llvm-project/pull/73989.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
index 153bac34986ec1c..25620129b38e145 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
@@ -339,7 +339,7 @@ bool RISCVLegalizerInfo::legalizeVAStart(MachineInstr &MI,
LLT AddrTy = MIRBuilder.getMRI()->getType(MI.getOperand(0).getReg());
auto FINAddr = MIRBuilder.buildFrameIndex(AddrTy, FI);
assert(MI.hasOneMemOperand());
- MIRBuilder.buildStore(MI.getOperand(0).getReg(), FINAddr,
+ MIRBuilder.buildStore(FINAddr, MI.getOperand(0).getReg(),
*MI.memoperands()[0]);
MI.eraseFromParent();
return true;
|
|
Add test case that I shared offline? |
llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-vastart-rv64.mir
Outdated
Show resolved
Hide resolved
| name: test_va_start | ||
| tracksRegLiveness: true | ||
| fixedStack: | ||
| - { id: 0, size: 4, alignment: 16, isImmutable: true } |
There was a problem hiding this comment.
What is the reason that alignment is 16 here but 4 in the stack below?
There was a problem hiding this comment.
The entry in fixedStack: is from the call to createFixedObject in the call lowering code. The stack pointer is 16 byte aligned when a function starts so the alignment is 16.
The entry in thestack: section is from the alloca which didn't make an explicit alignment request so got the ABI alignment for a pointer. It's location on the stack isn't decided yet so we only record the requested size and alignment. It's location will be assigned later by the prolog/epilog inserter pass.
Both entries say id: 0 but they are different objects since one is in fixedStack and one is in stack.
There was a problem hiding this comment.
I have a question that does not block this PR:
got the ABI alignment for a pointer.
stack pointer is 16 byte aligned when a function starts so the alignment is 16
Why is the ABI alignment for a pointer different than the stack pointer 16 byte alignment at function start?
There was a problem hiding this comment.
The ABI alignment for any legal type on RISC-V is the size of the type. So pointers are aligned to XLEN/8. i8 is aligned to 1 byte, i16 is aligned to 2 bytes, i32 is aligned to 4 bytes, etc.
The are some types in the psABI that are defined to have 16 byte alignment like long double and __int128. https://github.com/riscv-non-isa/riscv-elf-psabi-doc
The stack pointer alignment is a contract between all functions so that the code generation for any function can assume the stack pointer is 16 byte alignment. Otherwise we would need to emit code to align the stack pointer before we could put a long double or __int128 on the stack. When we generate code for a function, we know the stack started off 16 byte aligned. When we allocate things on the stack, we make sure that we always pad if needed to keep the stack pointer 16 byte aligned. This ensures the stack pointer is 16 byte aligned for any function calls.
…eVAStart. We need to store the frame index to the location pointed to by the VASTART, not the other way around. Apparently we're missing tests for G_VASTART so no tests changed.
|
Ping |
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py | ||
| # RUN: llc -mtriple=riscv32 -run-pass=legalizer %s -o - | FileCheck --check-prefix=RV32 %s | ||
|
|
||
| --- | |
There was a problem hiding this comment.
You can drop IR and remove name from stack.
We need to store the frame index to the location pointed to by the VASTART, not the other way around.
Apparently we're missing tests for G_VASTART so no tests changed.