Skip to content

[RISCV][GISel] Reverse the operands the buildStore created in legalizeVAStart. #73989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 8, 2023
Merged

[RISCV][GISel] Reverse the operands the buildStore created in legalizeVAStart. #73989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edefd1f
[RISCV][GISel] Reverse the operands the buildStore created in legaliz…
topperc Nov 30, 2023
1be010c
fixup! add tests.
topperc Dec 1, 2023
8e85a26
fixup! drop lines from test.
topperc Dec 5, 2023
c5d8d76
fixup! simplify test even more.
topperc Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,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;
Expand Down
29 changes: 29 additions & 0 deletions llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-vastart-rv32.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# 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

---
name: test_va_start
tracksRegLiveness: true
fixedStack:
- { id: 0, size: 4, alignment: 16, isImmutable: true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason that alignment is 16 here but 4 in the stack below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stack:
- { id: 0, size: 4, alignment: 4 }
machineFunctionInfo:
varArgsFrameIndex: -1
varArgsSaveSize: 0
body: |
bb.1:
liveins: $x10, $x11, $x12, $x13, $x14, $x15, $x16, $x17

; RV32-LABEL: name: test_va_start
; RV32: liveins: $x10, $x11, $x12, $x13, $x14, $x15, $x16, $x17
; RV32-NEXT: {{ $}}
; RV32-NEXT: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0
; RV32-NEXT: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
; RV32-NEXT: G_STORE [[FRAME_INDEX]](p0), [[FRAME_INDEX1]](p0) :: (store (s32))
; RV32-NEXT: PseudoRET
%8:_(p0) = G_FRAME_INDEX %stack.0
G_VASTART %8(p0) :: (store (s32))
PseudoRET

...
29 changes: 29 additions & 0 deletions llvm/test/CodeGen/RISCV/GlobalISel/legalizer/legalize-vastart-rv64.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv64 -run-pass=legalizer %s -o - | FileCheck --check-prefix=RV64 %s

---
name: test_va_start
tracksRegLiveness: true
fixedStack:
- { id: 0, size: 8, alignment: 16, isImmutable: true }
stack:
- { id: 0, size: 8, alignment: 8 }
machineFunctionInfo:
varArgsFrameIndex: -1
varArgsSaveSize: 0
body: |
bb.1:
liveins: $x10, $x11, $x12, $x13, $x14, $x15, $x16, $x17

; RV64-LABEL: name: test_va_start
; RV64: liveins: $x10, $x11, $x12, $x13, $x14, $x15, $x16, $x17
; RV64-NEXT: {{ $}}
; RV64-NEXT: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0
; RV64-NEXT: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
; RV64-NEXT: G_STORE [[FRAME_INDEX]](p0), [[FRAME_INDEX1]](p0) :: (store (s64))
; RV64-NEXT: PseudoRET
%8:_(p0) = G_FRAME_INDEX %stack.0
G_VASTART %8(p0) :: (store (s64))
PseudoRET

...