[RISCV][NFC] refactor CFI emitting #114227

dlav-sc · 2024-10-30T13:25:47Z

This patch refactor PR #110810 to remove code duplication.

llvmbot · 2024-10-30T13:26:20Z

@llvm/pr-subscribers-backend-risc-v

Author: None (dlav-sc)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/114227.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+94-98)
(modified) llvm/lib/Target/RISCV/RISCVFrameLowering.h (+3-2)

diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index 6375300117090f..b94dd031186356 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -27,6 +27,88 @@
 
 using namespace llvm;
 
+static unsigned getCaleeSavedRVVNumRegs(const Register &BaseReg) {
+  return RISCV::VRRegClass.contains(BaseReg)     ? 1
+         : RISCV::VRM2RegClass.contains(BaseReg) ? 2
+         : RISCV::VRM4RegClass.contains(BaseReg) ? 4
+                                                 : 8;
+}
+
+static MCRegister getRVVBaseRegister(const RISCVRegisterInfo &TRI, const Register &Reg) {
+  MCRegister BaseReg = TRI.getSubReg(Reg, RISCV::sub_vrm1_0);
+  // If it's not a grouped vector register, it doesn't have subregister, so
+  // the base register is just itself.
+  if (BaseReg == RISCV::NoRegister)
+    BaseReg = Reg;
+  return BaseReg;
+}
+
+namespace {
+
+struct CFIRestoreRegisterEmitter {
+  CFIRestoreRegisterEmitter(MachineFunction &, const RISCVSubtarget &) {};
+
+  void emit(MachineFunction &MF, MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI, const RISCVRegisterInfo &RI, const RISCVInstrInfo &TII, const DebugLoc &DL, const CalleeSavedInfo &CS) const {
+    Register Reg = CS.getReg();
+    unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createRestore(
+        nullptr, RI.getDwarfRegNum(Reg, true)));
+    BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
+        .addCFIIndex(CFIIndex)
+        .setMIFlag(MachineInstr::FrameDestroy);
+  }
+};
+
+class CFIStoreRegisterEmitter {
+  MachineFrameInfo &MFI;
+  
+  public:
+  CFIStoreRegisterEmitter(MachineFunction &MF, const RISCVSubtarget &) : MFI{MF.getFrameInfo()} {};
+  
+  void emit(MachineFunction &MF, MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI, const RISCVRegisterInfo &RI, const RISCVInstrInfo &TII, const DebugLoc &DL, const CalleeSavedInfo &CS) const {
+    int FrameIdx = CS.getFrameIdx();
+    int64_t Offset = MFI.getObjectOffset(FrameIdx);
+    Register Reg = CS.getReg();
+    unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+        nullptr, RI.getDwarfRegNum(Reg, true), Offset));
+    BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
+        .addCFIIndex(CFIIndex)
+        .setMIFlag(MachineInstr::FrameSetup);
+  }
+};
+
+class CFIRestoreRVVRegisterEmitter {
+  const llvm::RISCVRegisterInfo *TRI;
+  
+  public:
+  CFIRestoreRVVRegisterEmitter(MachineFunction &, const RISCVSubtarget &STI) : TRI{STI.getRegisterInfo()} {};
+
+  void emit(MachineFunction &MF, MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI, const RISCVRegisterInfo &RI, const RISCVInstrInfo &TII, const DebugLoc &DL, const CalleeSavedInfo &CS) const {
+    MCRegister BaseReg = getRVVBaseRegister(*TRI, CS.getReg());
+    unsigned NumRegs = getCaleeSavedRVVNumRegs(CS.getReg());
+    for (unsigned i = 0; i < NumRegs; ++i) {
+      unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createRestore(
+          nullptr, RI.getDwarfRegNum(BaseReg + i, true)));
+      BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
+          .addCFIIndex(CFIIndex)
+          .setMIFlag(MachineInstr::FrameDestroy);
+    }
+  }
+};
+
+}
+
+template <typename Emitter>
+void RISCVFrameLowering::emitCFIForCSI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI, const SmallVector<CalleeSavedInfo, 8> &CSI) const {
+  MachineFunction *MF = MBB.getParent();
+  const RISCVRegisterInfo *RI = STI.getRegisterInfo();
+  const RISCVInstrInfo *TII = STI.getInstrInfo();
+  DebugLoc DL = MBB.findDebugLoc(MBBI);
+  
+  Emitter E{*MF, STI}; 
+  for (const auto &CS : CSI)
+    E.emit(*MF, MBB, MBBI, *RI, *TII, DL, CS);
+}
+
 static Align getABIStackAlignment(RISCVABI::ABI ABI) {
   if (ABI == RISCVABI::ABI_ILP32E)
     return Align(4);
@@ -607,16 +689,7 @@ void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
         .addCFIIndex(CFIIndex)
         .setMIFlag(MachineInstr::FrameSetup);
 
-    for (const auto &Entry : getPushOrLibCallsSavedInfo(MF, CSI)) {
-      int FrameIdx = Entry.getFrameIdx();
-      int64_t Offset = MFI.getObjectOffset(FrameIdx);
-      Register Reg = Entry.getReg();
-      unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
-          nullptr, RI->getDwarfRegNum(Reg, true), Offset));
-      BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-          .addCFIIndex(CFIIndex)
-          .setMIFlag(MachineInstr::FrameSetup);
-    }
+    emitCFIForCSI<CFIStoreRegisterEmitter>(MBB, MBBI, getPushOrLibCallsSavedInfo(MF, CSI));
   }
 
   // FIXME (note copied from Lanai): This appears to be overallocating.  Needs
@@ -658,16 +731,7 @@ void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
         .addCFIIndex(CFIIndex)
         .setMIFlag(MachineInstr::FrameSetup);
 
-    for (const auto &Entry : getPushOrLibCallsSavedInfo(MF, CSI)) {
-      int FrameIdx = Entry.getFrameIdx();
-      int64_t Offset = MFI.getObjectOffset(FrameIdx);
-      Register Reg = Entry.getReg();
-      unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
-          nullptr, RI->getDwarfRegNum(Reg, true), Offset));
-      BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-          .addCFIIndex(CFIIndex)
-          .setMIFlag(MachineInstr::FrameSetup);
-    }
+    emitCFIForCSI<CFIStoreRegisterEmitter>(MBB, MBBI, getPushOrLibCallsSavedInfo(MF, CSI));
   }
 
   if (StackSize != 0) {
@@ -694,20 +758,7 @@ void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
 
   // Iterate over list of callee-saved registers and emit .cfi_offset
   // directives.
-  for (const auto &Entry : getUnmanagedCSI(MF, CSI)) {
-    int FrameIdx = Entry.getFrameIdx();
-    if (FrameIdx >= 0 &&
-        MFI.getStackID(FrameIdx) == TargetStackID::ScalableVector)
-      continue;
-
-    int64_t Offset = MFI.getObjectOffset(FrameIdx);
-    Register Reg = Entry.getReg();
-    unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
-        nullptr, RI->getDwarfRegNum(Reg, true), Offset));
-    BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-        .addCFIIndex(CFIIndex)
-        .setMIFlag(MachineInstr::FrameSetup);
-  }
+  emitCFIForCSI<CFIStoreRegisterEmitter>(MBB, MBBI, getUnmanagedCSI(MF, CSI)); 
 
   // Generate new FP.
   if (hasFP(MF)) {
@@ -895,7 +946,7 @@ void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
           .setMIFlag(MachineInstr::FrameDestroy);
     }
 
-    emitCalleeSavedRVVEpilogCFI(MBB, LastFrameDestroy);
+    emitCFIForCSI<CFIRestoreRVVRegisterEmitter>(MBB, LastFrameDestroy, getRVVCalleeSavedInfo(MF, CSI));
   }
 
   if (FirstSPAdjustAmount) {
@@ -960,14 +1011,7 @@ void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
   }
 
   // Recover callee-saved registers.
-  for (const auto &Entry : getUnmanagedCSI(MF, CSI)) {
-    Register Reg = Entry.getReg();
-    unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createRestore(
-        nullptr, RI->getDwarfRegNum(Reg, true)));
-    BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-        .addCFIIndex(CFIIndex)
-        .setMIFlag(MachineInstr::FrameDestroy);
-  }
+  emitCFIForCSI<CFIRestoreRegisterEmitter>(MBB, MBBI, getUnmanagedCSI(MF, CSI));
 
   bool ApplyPop = RVFI->isPushable(MF) && MBBI != MBB.end() &&
                   MBBI->getOpcode() == RISCV::CM_POP;
@@ -976,7 +1020,7 @@ void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
     // space. Align the stack size down to a multiple of 16. This is needed for
     // RVE.
     // FIXME: Can we increase the stack size to a multiple of 16 instead?
-    uint64_t Spimm = std::min(alignDown(StackSize, 16), (uint64_t)48);
+    uint64_t Spimm = std::min(alignDown(StackSize, 16), static_cast<uint64_t>(48));
     MBBI->getOperand(1).setImm(Spimm);
     StackSize -= Spimm;
 
@@ -988,15 +1032,8 @@ void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
     if (NextI == MBB.end() || NextI->getOpcode() != RISCV::PseudoRET) {
       ++MBBI;
 
-      for (const auto &Entry : getPushOrLibCallsSavedInfo(MF, CSI)) {
-        Register Reg = Entry.getReg();
-        unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createRestore(
-            nullptr, RI->getDwarfRegNum(Reg, true)));
-        BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
-            .addCFIIndex(CFIIndex)
-            .setMIFlag(MachineInstr::FrameDestroy);
-      }
-
+      emitCFIForCSI<CFIRestoreRegisterEmitter>(MBB, MBBI, getPushOrLibCallsSavedInfo(MF, CSI));
+      
       // Update CFA offset. After CM_POP SP should be equal to CFA, so CFA offset
       // should be a zero.
       unsigned CFIIndex =
@@ -1006,7 +1043,7 @@ void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
           .setMIFlag(MachineInstr::FrameDestroy);
     }
   }
-  
+
   // Deallocate stack if StackSize isn't a zero yet
   if (StackSize != 0)
     deallocateStack(MF, MBB, MBBI, DL, StackSize, 0);
@@ -1696,22 +1733,6 @@ bool RISCVFrameLowering::spillCalleeSavedRegisters(
   return true;
 }
 
-static unsigned getCaleeSavedRVVNumRegs(const Register &BaseReg) {
-  return RISCV::VRRegClass.contains(BaseReg)     ? 1
-         : RISCV::VRM2RegClass.contains(BaseReg) ? 2
-         : RISCV::VRM4RegClass.contains(BaseReg) ? 4
-                                                 : 8;
-}
-
-static MCRegister getRVVBaseRegister(const RISCVRegisterInfo &TRI, const Register &Reg) {
-  MCRegister BaseReg = TRI.getSubReg(Reg, RISCV::sub_vrm1_0);
-  // If it's not a grouped vector register, it doesn't have subregister, so
-  // the base register is just itself.
-  if (BaseReg == RISCV::NoRegister)
-    BaseReg = Reg;
-  return BaseReg;
-}
-
 void RISCVFrameLowering::emitCalleeSavedRVVPrologCFI(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MI, bool HasFP) const {
   MachineFunction *MF = MBB.getParent();
@@ -1737,39 +1758,14 @@ void RISCVFrameLowering::emitCalleeSavedRVVPrologCFI(
   for (auto &CS : RVVCSI) {
     // Insert the spill to the stack frame.
     int FI = CS.getFrameIdx();
-    if (FI >= 0 && MFI.getStackID(FI) == TargetStackID::ScalableVector) {
-      MCRegister BaseReg = getRVVBaseRegister(TRI, CS.getReg());
-      unsigned NumRegs = getCaleeSavedRVVNumRegs(CS.getReg());
-      for (unsigned i = 0; i < NumRegs; ++i) {
-        unsigned CFIIndex = MF->addFrameInst(createDefCFAOffset(
-            TRI, BaseReg + i, -FixedSize, MFI.getObjectOffset(FI) / 8 + i));
-        BuildMI(MBB, MI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
-            .addCFIIndex(CFIIndex)
-            .setMIFlag(MachineInstr::FrameSetup);
-      }
-    }
-  }
-}
-
-void RISCVFrameLowering::emitCalleeSavedRVVEpilogCFI(
-    MachineBasicBlock &MBB, MachineBasicBlock::iterator MI) const {
-  MachineFunction *MF = MBB.getParent();
-  const MachineFrameInfo &MFI = MF->getFrameInfo();
-  const RISCVRegisterInfo *RI = STI.getRegisterInfo();
-  const TargetInstrInfo &TII = *STI.getInstrInfo();
-  const RISCVRegisterInfo &TRI = *STI.getRegisterInfo();
-  DebugLoc DL = MBB.findDebugLoc(MI);
-
-  const auto &RVVCSI = getRVVCalleeSavedInfo(*MF, MFI.getCalleeSavedInfo());
-  for (auto &CS : RVVCSI) {
     MCRegister BaseReg = getRVVBaseRegister(TRI, CS.getReg());
     unsigned NumRegs = getCaleeSavedRVVNumRegs(CS.getReg());
     for (unsigned i = 0; i < NumRegs; ++i) {
-      unsigned CFIIndex = MF->addFrameInst(MCCFIInstruction::createRestore(
-          nullptr, RI->getDwarfRegNum(BaseReg + i, true)));
+      unsigned CFIIndex = MF->addFrameInst(createDefCFAOffset(
+          TRI, BaseReg + i, -FixedSize, MFI.getObjectOffset(FI) / 8 + i));
       BuildMI(MBB, MI, DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex)
-          .setMIFlag(MachineInstr::FrameDestroy);
+          .setMIFlag(MachineInstr::FrameSetup);
     }
   }
 }
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.h b/llvm/lib/Target/RISCV/RISCVFrameLowering.h
index 31857fea8ea0a9..6be905b12623fb 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.h
@@ -91,11 +91,12 @@ class RISCVFrameLowering : public TargetFrameLowering {
   void emitCalleeSavedRVVPrologCFI(MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator MI,
                                    bool HasFP) const;
-  void emitCalleeSavedRVVEpilogCFI(MachineBasicBlock &MBB,
-                                   MachineBasicBlock::iterator MI) const;
   void deallocateStack(MachineFunction &MF, MachineBasicBlock &MBB,
                        MachineBasicBlock::iterator MBBI, const DebugLoc &DL,
                        uint64_t &StackSize, int64_t CFAOffset) const;
+  template <typename Emitter>
+  void emitCFIForCSI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
+                     const SmallVector<CalleeSavedInfo, 8> &CSI) const;
 
   std::pair<int64_t, Align>
   assignRVVStackObjectOffsets(MachineFunction &MF) const;

github-actions · 2024-10-30T16:32:55Z

✅ With the latest revision this PR passed the C/C++ code formatter.

lenary

This cleanup is going in a nice direction, I think.

A few suggestions/questions below.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

lenary

LGTM

dlav-sc · 2024-11-13T12:30:11Z

LGTM

I've tried to make emitters for RVV 081ab88. However, I'm not sure that they make code more clear to be honest, maybe it would be better to leave emitCalleeSavedRVVPrologCFI and emitCalleeSavedRVVPEpilogCFI as they are.

lenary · 2024-11-13T12:54:57Z

LGTM

I've tried to make emitters for RVV 081ab88. However, I'm not sure that they make code more clear to be honest, maybe it would be better to leave emitCalleeSavedRVVPrologCFI and emitCalleeSavedRVVPEpilogCFI as they are.

I think I agree. I was happy enough without the emitters for RVV (I think your push of that commit raced my LGTM)

dlav-sc · 2024-11-14T15:44:57Z

LGTM

I've tried to make emitters for RVV 081ab88. However, I'm not sure that they make code more clear to be honest, maybe it would be better to leave emitCalleeSavedRVVPrologCFI and emitCalleeSavedRVVPEpilogCFI as they are.

I think I agree. I was happy enough without the emitters for RVV (I think your push of that commit raced my LGTM)

I've rolled back to the approved version

topperc · 2024-11-14T18:02:28Z

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

+namespace {
+
+class CFISaveRegisterEmitter {
+  MachineFunction &m_MF;


LLVM doesn't use m_ and we don't generally have underscores in variable names.

topperc

LGTM

dlav-sc · 2024-11-18T09:26:43Z

Thank you for the review!

llvmbot added the backend:RISC-V label Oct 30, 2024

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-epilog branch from 2c44cf1 to 107a646 Compare October 30, 2024 13:26

dlav-sc mentioned this pull request Oct 30, 2024

[RISCV][CFI] add function epilogue cfi information #110810

Merged

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-epilog branch 2 times, most recently from 5126965 to 255db2d Compare October 30, 2024 16:23

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch from 825bc96 to 3c52c68 Compare October 30, 2024 16:29

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-epilog branch from 255db2d to f49dafe Compare October 30, 2024 16:33

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch 2 times, most recently from 19f5cc5 to 045774c Compare October 30, 2024 16:45

lenary reviewed Oct 30, 2024

View reviewed changes

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-epilog branch 8 times, most recently from 9631627 to a2d62de Compare November 5, 2024 16:11

Base automatically changed from users/dlav-sc/riscv-cfi-epilog to main November 5, 2024 21:20

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch from 045774c to 5a5c5f9 Compare November 5, 2024 21:30

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch 2 times, most recently from e816236 to 80c66c5 Compare November 13, 2024 10:41

lenary approved these changes Nov 13, 2024

View reviewed changes

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch from 80c66c5 to 582bde4 Compare November 14, 2024 15:36

[RISCV][NFC] refactor CFI emitting

ff13b27

dlav-sc force-pushed the users/dlav-sc/riscv-cfi-refactoring branch from 582bde4 to ff13b27 Compare November 14, 2024 15:43

lenary approved these changes Nov 14, 2024

View reviewed changes

topperc reviewed Nov 14, 2024

View reviewed changes

rename class members

900ac4c

topperc approved these changes Nov 15, 2024

View reviewed changes

dlav-sc merged commit 0c04d43 into main Nov 18, 2024
8 checks passed

dlav-sc deleted the users/dlav-sc/riscv-cfi-refactoring branch November 18, 2024 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV][NFC] refactor CFI emitting #114227

[RISCV][NFC] refactor CFI emitting #114227

Uh oh!

dlav-sc commented Oct 30, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 30, 2024

Uh oh!

github-actions bot commented Oct 30, 2024 •

edited

Loading

Uh oh!

lenary left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lenary left a comment

Uh oh!

dlav-sc commented Nov 13, 2024

Uh oh!

lenary commented Nov 13, 2024 •

edited

Loading

Uh oh!

dlav-sc commented Nov 14, 2024

Uh oh!

topperc Nov 14, 2024

Uh oh!

dlav-sc Nov 15, 2024

Uh oh!

topperc left a comment

Uh oh!

Uh oh!

dlav-sc commented Nov 18, 2024

Uh oh!

Uh oh!

[RISCV][NFC] refactor CFI emitting #114227

[RISCV][NFC] refactor CFI emitting #114227

Uh oh!

Conversation

dlav-sc commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 30, 2024

Uh oh!

github-actions bot commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

dlav-sc commented Nov 13, 2024

Uh oh!

lenary commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlav-sc commented Nov 14, 2024

Uh oh!

topperc Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

dlav-sc Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlav-sc commented Nov 18, 2024

Uh oh!

Uh oh!

dlav-sc commented Oct 30, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024 •

edited

Loading

lenary commented Nov 13, 2024 •

edited

Loading