This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use max pushed register to get pushed register number.
ClosedPublic

Authored by fakepaper56 on Jul 27 2023, 2:49 AM.

Details

Summary

Previously we used the number of registers needed saved and pushable as the
number of pushed registers. We also use pushed register number to caculate
the stack size. It is not correct because Zcmp pushes registers from $ra to the
max register needed saved and there is no gurantee that the needed saved
registers are a sequenced list from $ra.

There is an example about that. PushPopRegs should be 6 (ra,s0 - s4)= instead of 1.

; llc -mtriple=riscv32 -mattr=+zcmp
define void @foo() {
entry:
; Old:    .cfi_def_cfa_offset 16
; New:    .cfi_def_cfa_offset 32
  tail call void asm sideeffect "li s4, 0", "~{s4}"()
  ret void
}

Diff Detail

Event Timeline

fakepaper56 created this revision.Jul 27 2023, 2:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 2:49 AM
fakepaper56 requested review of this revision.Jul 27 2023, 2:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 2:49 AM
Jim added inline comments.Jul 27 2023, 6:30 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
1329

Should we ues PushPopNumRegs?

fakepaper56 added inline comments.Jul 27 2023, 7:03 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
1329

If we want to change the original name, I prefer PushedRegNum more.

Jim accepted this revision.Jul 27 2023, 7:56 PM

LGTM, but wait other review's approve.

This revision is now accepted and ready to land.Jul 27 2023, 7:56 PM

Rename PushPopRegs to PushedRegNum.

KYG added inline comments.Jul 31 2023, 3:41 AM
llvm/test/CodeGen/RISCV/callee-saved-gprs.ll
1994–1995

How about mentioning that it's testing stack size calculation?

Address Kai's comment.

fakepaper56 added a comment.EditedAug 2 2023, 7:59 PM

Claim: If the revision can not get the second approve before 8/7, I will land it.

Is this also needed in LLVM 17?

kito-cheng accepted this revision.Aug 2 2023, 11:33 PM

LGTM, and I think this need to backport to LLVM 17

This revision was landed with ongoing or failed builds.Aug 2 2023, 11:49 PM
This revision was automatically updated to reflect the committed changes.

I think this need to backport to LLVM 17

I had created issue https://github.com/llvm/llvm-project/issues/64367.