Rebased save csr in ra #131845

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

Jump to bottom

Draft

mgudim wants to merge 2 commits into llvm:main from mgudim:rebased_save_csr_in_ra

Contributor

mgudim commented Mar 18, 2025

No description provided.

WIP

28731f5

mgudim mentioned this pull request

[MC][AsmPrinter] Introduce llvm_reg_offset pseudo cfi instruction. #125104

Closed

michaelmaitland reviewed

View reviewed changes

Contributor

michaelmaitland left a comment

Thanks for posting this. It really helps to see the full picture here because the additional context is necessary in giving review.

I'd really like to see you split this into a series of patches, all stacked on top of eachother. That way, we can review in small chunks but refer to the stacked components for context.

Please see #119359 for an example. The stacking of PRs makes the job as a reviewer much easier, and it will allow me to give feedback faster.

I left some comments here. Some are questions I hope you can answer here, and some are comments I hope can be useful as you split into stacked patches. Lastly, I think some of the changes in this PR have already made it in (like the reaching def stuff)

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                  MachineBasicBlock::iterator InsertPos =
+                      Info.InsertAfterMI ? ++(Info.InsertAfterMI->getIterator())
+                                         : Info.MBB->begin();
+                  BuildMI(*Info.MBB, InsertPos, Info.DL,

Contributor

michaelmaitland Mar 18, 2025

It looks like we are emitting CFI instruction here. Why did this not occur in trackRegisterandEmitCFIs?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                                                                BitVector &SavedRegs) const {
+                const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
+                // Resize before the early returns. Some backends expect that

Contributor

michaelmaitland Mar 18, 2025

What do you mean some backends expect this? IIUC, there is no other subtarget that has this function and this function for the RISC-V backend.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                                                            BitVector &SavedRegs,
+                                                            RegScavenger *RS) const {
+                const auto &ST = MF.getSubtarget<RISCVSubtarget>();
+                determineMustCalleeSaves(MF, SavedRegs);

Contributor

michaelmaitland Mar 18, 2025 •

edited

Loading

It looks like we're doing additional logic that did not exist before in the case that !ST.doCSRSavesInRA(). All the logic before

if (hasFP(MF)) {
    SavedRegs.set(RISCV::X1);
    SavedRegs.set(RISCV::X8);
  }

in determineMustCalleeSaves is new. Is all of that additional logic desired when !doCSRSavesInRA?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

                 if (hasFP(MF)) {
-                  SavedRegs.set(RAReg);
-                  SavedRegs.set(FPReg);
+                  SavedRegs.set(RISCV::X1);

Contributor

michaelmaitland Mar 18, 2025

Why did this code change? Isn't it the same?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                return;
+              }
+              int RISCVFrameLowering::getInitialCFAOffset(const MachineFunction &MF) const {

Contributor

michaelmaitland Mar 18, 2025

Is this used? I assume its used somewhere in the CFIInserter already. I'm trying to understand why we didn't need to implement these before wanting to do SaveCSRInRA.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                unsigned CFIIndex;
+              };
+              static void trackRegisterAndEmitCFIs(

Contributor

michaelmaitland Mar 18, 2025

Can you help me understand what this function is doing. Why is it needed? How does it work?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+                if (Defs.empty()) {
+                  // it's a live-in register at the entry block.
+                  // unsigned CFIIndex =
+                  // MF.addFrameInst(MCCFIInstruction::createSameValue(nullptr,

Contributor

michaelmaitland Mar 18, 2025

Why is this commented out? Do we need it?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

+                const RISCVRegisterInfo &TRI = *Subtarget.getRegisterInfo();
+                const RISCVFrameLowering &TFI = *Subtarget.getFrameLowering();
+                SmallVector<MachineInstr *, 4> RestorePoints;

Contributor

michaelmaitland Mar 18, 2025

Why didn't we do this in prologepilog when we inserted the CFI instructions?


          [RISCV][WIP] Let RA do the CSR saves.

ef9ee46

We turn the problem of saving and restoring callee-saved registers efficiently into a register allocation problem. This has the advantage that the register allocator can essentialy do shrink-wrapping on per register basis. Currently, shrink-wrapping pass saves all CSR in the same place which may be suboptimal. Also, improvements to register allocation / coalescing will translate to improvements in shrink-wrapping.

In finalizeLowering() we copy all callee-saved registers from a physical register to a virtual one. In all return blocks we copy do the reverse.

mgudim force-pushed the rebased_save_csr_in_ra branch from f843602 to ef9ee46 Compare

March 18, 2025 17:58

michaelmaitland reviewed

View reviewed changes

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

@@ @@ -582,6 +582,8 @@ void RISCVPassConfig::addPreEmitPass2() { @@
                 addPass(createUnpackMachineBundles([&](const MachineFunction &MF) {
                   return MF.getFunction().getParent()->getModuleFlag("kcfi");
                 }));
+                addPass(createCFIInstrInserter());

Contributor

michaelmaitland Mar 18, 2025

Why did we previously not need this pass? Didn't we used to emit CFI Instrs?

Contributor Author

mgudim commented Mar 18, 2025

Thanks for posting this. It really helps to see the full picture here because the additional context is necessary in giving review.

I'd really like to see you split this into a series of patches, all stacked on top of eachother. That way, we can review in small chunks but refer to the stacked components for context.

Please see #119359 for an example. The stacking of PRs makes the job as a reviewer much easier, and it will allow me to give feedback faster.

I left some comments here. Some are questions I hope you can answer here, and some are comments I hope can be useful as you split into stacked patches. Lastly, I think some of the changes in this PR have already made it in (like the reaching def stuff)

Sure, I'll work on splitting everything into commits and stack them all on top of each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet