This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add missed fcsr spill and restore in interrupt
Needs ReviewPublic

Authored by Yunzezhu on Aug 21 2023, 1:22 AM.

Details

Summary

According to gcc, fcsr should be spill and restore in interrupt, which is missed in llvm.
This patch added fcsr spill and restore when has interrupt and standard extension f on.

Diff Detail

Unit TestsFailed

Event Timeline

Yunzezhu created this revision.Aug 21 2023, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 1:22 AM
Yunzezhu requested review of this revision.Aug 21 2023, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 1:22 AM
wangpc added a comment.EditedAug 21 2023, 9:15 PM

I am not against this patch, just one thought: is it possible that we add FCSR as a RISCVReg in RISCVRegisterInfo.td (new RegisterClass may be needed), and add it to CSR_Interrupt in RISCVCallingConv.td? The spill/restore can be done by CSRRS for this kind of CSRs.
This may reduce the code if we need spill/restore more CSRs in the future.

I am not against this patch, just one thought: is it possible that we add FCSR as a RISCVReg in RISCVRegisterInfo.td (new RegisterClass may be needed), and add it to CSR_Interrupt in RISCVCallingConv.td? The spill/restore can be done by CSRRS for this kind of CSRs.
This may reduce the code if we need spill/restore more CSRs in the future.

Will that need to create a virtual register and scavenge a GPR since we can't load/store a GPR from memory directly?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
752

Can we use the WriteFRM and ReadFRM pseudo instructions?

752

Nevermind. it's fcsr not just frm

I am not against this patch, just one thought: is it possible that we add FCSR as a RISCVReg in RISCVRegisterInfo.td (new RegisterClass may be needed), and add it to CSR_Interrupt in RISCVCallingConv.td? The spill/restore can be done by CSRRS for this kind of CSRs.
This may reduce the code if we need spill/restore more CSRs in the future.

Will that need to create a virtual register and scavenge a GPR since we can't load/store a GPR from memory directly?

I posted the patch D158492 though I'm not sure it's worthy as it increases stack size and adds complexity to code.

asb added a comment.Sep 5 2023, 3:58 AM

Relevant context would be this clarification to the riscv-c-api-doc. It doesn't look like anyone has given much thought to vectors so far.

I need to take a look at the patch with the alternate approach, but so I don't forget it: this patch needs to be extended to handle the zfinx case as well (with appropriate test too of course).