This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add CSR RegisterClass and save/restore fcsr in interrupt
Needs ReviewPublic

Authored by wangpc on Aug 22 2023, 1:06 AM.

Details

Summary

We add a new RegisterClass to represent CSRs and modify
storeRegToStackSlot/loadRegFromStackSlot so that we can
save/restore these CSRs.

This is an alternative of D158396

Diff Detail

Event Timeline

wangpc created this revision.Aug 22 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 1:06 AM
wangpc requested review of this revision.Aug 22 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 1:06 AM
wangpc updated this revision to Diff 552256.Aug 22 2023, 1:18 AM

Fix wrong restore dst reg.

wangpc updated this revision to Diff 552258.Aug 22 2023, 1:26 AM

Fix paste error.

wangpc updated this revision to Diff 552269.Aug 22 2023, 2:05 AM

CSR restore should be ahead.

Jim added inline comments.Aug 22 2023, 2:39 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
379–380

If we put FCSR at first in CSR_XLEN_F32_Interrupt/CSR_XLEN_F64_Interrupt, is it still need to depend on save or restore to determinate the order of CSR inserted.

577

Typo. IsRestore.

wangpc updated this revision to Diff 552279.Aug 22 2023, 2:51 AM
wangpc marked an inline comment as done.

Fix typo.

wangpc marked an inline comment as done.Aug 22 2023, 2:51 AM
wangpc added inline comments.
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
379–380

Yes. We should save FCSR after saving t0 and restore it before restoring t0 since we are using t0 as a temporary register.

Gentle ping. :-)

craig.topper added inline comments.Aug 28 2023, 7:59 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
549

This seems tricky to me to hardcode a GPR in here. This makes the code only work in certain scenarios. Do other targets do anything like this?

wangpc added inline comments.Aug 30 2023, 1:12 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
549

This seems tricky to me to hardcode a GPR in here.

We just need a GPR to store the value of CSRs.

This makes the code only work in certain scenarios.

I don't get it. This code is only for saving CSRs.

Do other targets do anything like this?

No, I haven't find any target doing this.

craig.topper added inline comments.Aug 30 2023, 8:31 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
549

This seems tricky to me to hardcode a GPR in here.

We just need a GPR to store the value of CSRs.

This makes the code only work in certain scenarios.

I don't get it. This code is only for saving CSRs.

It only works in the prolog and epilogue where that GPR is available. loadRegFromStackSlot and storeRegToStackSlot are normally also used for spills and reloads, but that wouldn’t work if we ever needed that for CSRs.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
600

This overlaps with FRM and FFLAGS. Probably at least deserves a comment about why they aren’t subregs.

asb added a comment.Sep 5 2023, 6:03 AM

Left a trivial comment. Not sure on this vs D158396 - the logic of the other patch does seem a bit easier to follow as you suggested in your comment there.

llvm/lib/Target/RISCV/RISCVCallingConv.td
34

We should probably mention fcsr in this comment (and the one for f64 below).

Left a trivial comment. Not sure on this vs D158396 - the logic of the other patch does seem a bit easier to follow as you suggested in your comment there.

I think it's more consistent than D158396 if consider CSRs as callee-saved register in one consistent place and infra.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
553

Probably it can refer to SPILL_CR like Pseudo in PPC target. Just use Pseudo here and expand later in eliminateFrameIndex where introduce internal virtual GPR, then it would be scavenged later

wangpc added inline comments.Oct 12 2023, 11:34 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
553

Thanks, I'll have a try!