Skip to content

[RISCV] Simplify tracking of tracking and encoding of push/pop in RISCVFrameLowering. NFC #129343

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.

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

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Mar 1, 2025

Previously we calculated the max register id. Then converted it number of registers and encoding. Then converted number of registers to stack size. Then saved number of registers, encoding, and stack size to MachineFunctionInfo.

This patch removes the calculation of max register id, and instead calculates the number of registers. The encoding is removed from MachineFunctionInfo in favor of converting the number of registers to encoding at the time of use.

…CVFrameLowering. NFC

Previously we calculated the max register id. Then converted it
number of registers and encoding. Then converted number of registers to
stack size. Then saved number of registers, encoding, and stack size
to MachineFunctionInfo.

This patch removes the calculation of max register id, and instead
calculates the number of registers. The encoding is removed from
MachineFunctionInfo in favor of converting the number of registers
to encoding at the time of use.
@topperc topperc requested a review from lenary March 1, 2025 00:27
@llvmbot
Copy link
Member

llvmbot commented Mar 1, 2025

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

Author: Craig Topper (topperc)

Changes

Previously we calculated the max register id. Then converted it number of registers and encoding. Then converted number of registers to stack size. Then saved number of registers, encoding, and stack size to MachineFunctionInfo.

This patch removes the calculation of max register id, and instead calculates the number of registers. The encoding is removed from MachineFunctionInfo in favor of converting the number of registers to encoding at the time of use.


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

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h (+9)
  • (modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+17-52)
  • (modified) llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h (-4)
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
index 185a5d8a9c9b2..d82ec0cb07a54 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
@@ -624,6 +624,15 @@ inline unsigned encodeRlist(MCRegister EndReg, bool IsRV32E = false) {
   }
 }
 
+inline static unsigned encodeRlistNumRegs(unsigned NumRegs) {
+  assert(NumRegs > 0 && NumRegs < 14 && NumRegs != 12 &&
+         "Unexpected number of registers");
+  if (NumRegs == 13)
+    --NumRegs;
+
+  return RLISTENCODE::RA + (NumRegs - 1);
+}
+
 inline static unsigned getStackAdjBase(unsigned RlistVal, bool IsRV64) {
   assert(RlistVal != RLISTENCODE::INVALID_RLIST &&
          "{ra, s0-s10} is not supported, s11 must be included.");
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index 8a18e681e1b85..bcc1909a5d195 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -332,51 +332,19 @@ getRestoreLibCallName(const MachineFunction &MF,
   return RestoreLibCalls[LibCallID];
 }
 
-// Return encoded value and register count for PUSH/POP instruction,
-// representing registers to store/load.
-static std::pair<unsigned, unsigned>
-getPushPopEncodingAndNum(const Register MaxReg) {
-  switch (MaxReg.id()) {
-  default:
-    llvm_unreachable("Unexpected Reg for Push/Pop Inst");
-  case RISCV::X27: /*s11*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S11, 13);
-  case RISCV::X25: /*s9*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S9, 11);
-  case RISCV::X24: /*s8*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S8, 10);
-  case RISCV::X23: /*s7*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S7, 9);
-  case RISCV::X22: /*s6*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S6, 8);
-  case RISCV::X21: /*s5*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S5, 7);
-  case RISCV::X20: /*s4*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S4, 6);
-  case RISCV::X19: /*s3*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S3, 5);
-  case RISCV::X18: /*s2*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S2, 4);
-  case RISCV::X9: /*s1*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0_S1, 3);
-  case FPReg: /*s0*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA_S0, 2);
-  case RAReg: /*ra*/
-    return std::make_pair(llvm::RISCVZC::RLISTENCODE::RA, 1);
-  }
-}
-
 // Get the max reg of Push/Pop for restoring callee saved registers.
-static Register getMaxPushPopReg(const std::vector<CalleeSavedInfo> &CSI) {
-  MCRegister MaxPushPopReg;
+static unsigned getNumPushPopRegs(const std::vector<CalleeSavedInfo> &CSI) {
+  unsigned NumPushPopRegs = 0;
   for (auto &CS : CSI) {
-    if (llvm::find_if(FixedCSRFIMap, [&](auto P) {
-          return P.first == CS.getReg();
-        }) != std::end(FixedCSRFIMap))
-      MaxPushPopReg = std::max(MaxPushPopReg.id(), CS.getReg().id());
+    auto *FII = llvm::find_if(FixedCSRFIMap,
+                              [&](auto P) { return P.first == CS.getReg(); });
+    if (FII != std::end(FixedCSRFIMap)) {
+      unsigned RegNum = std::distance(std::begin(FixedCSRFIMap), FII);
+      NumPushPopRegs = std::max(NumPushPopRegs, RegNum + 1);
+    }
   }
-  assert(MaxPushPopReg != RISCV::X26 && "x26 requires x27 to also be pushed");
-  return MaxPushPopReg;
+  assert(NumPushPopRegs != 12 && "x26 requires x27 to also be pushed");
+  return NumPushPopRegs;
 }
 
 // Return true if the specified function should have a dedicated frame
@@ -1790,14 +1758,10 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
 
   if (RVFI->isPushable(MF)) {
     // Determine how many GPRs we need to push and save it to RVFI.
-    Register MaxReg = getMaxPushPopReg(CSI);
-    if (MaxReg != RISCV::NoRegister) {
-      auto [RegEnc, PushedRegNum] = getPushPopEncodingAndNum(MaxReg);
+    unsigned PushedRegNum = getNumPushPopRegs(CSI);
+    if (PushedRegNum) {
       RVFI->setRVPushRegs(PushedRegNum);
       RVFI->setRVPushStackSize(alignTo((STI.getXLen() / 8) * PushedRegNum, 16));
-
-      // Use encoded number to represent registers to spill.
-      RVFI->setRVPushRlist(RegEnc);
     }
   }
 
@@ -1879,11 +1843,11 @@ bool RISCVFrameLowering::spillCalleeSavedRegisters(
     unsigned PushedRegNum = RVFI->getRVPushRegs();
     if (PushedRegNum > 0) {
       // Use encoded number to represent registers to spill.
-      int RegEnc = RVFI->getRVPushRlist();
+      unsigned RegEnc = RISCVZC::encodeRlistNumRegs(PushedRegNum);
       MachineInstrBuilder PushBuilder =
           BuildMI(MBB, MI, DL, TII.get(RISCV::CM_PUSH))
               .setMIFlag(MachineInstr::FrameSetup);
-      PushBuilder.addImm((int64_t)RegEnc);
+      PushBuilder.addImm(RegEnc);
       PushBuilder.addImm(0);
 
       for (unsigned i = 0; i < PushedRegNum; i++)
@@ -2032,8 +1996,9 @@ bool RISCVFrameLowering::restoreCalleeSavedRegisters(
 
   RISCVMachineFunctionInfo *RVFI = MF->getInfo<RISCVMachineFunctionInfo>();
   if (RVFI->isPushable(*MF)) {
-    int RegEnc = RVFI->getRVPushRlist();
-    if (RegEnc != llvm::RISCVZC::RLISTENCODE::INVALID_RLIST) {
+    unsigned PushedRegNum = RVFI->getRVPushRegs();
+    if (PushedRegNum > 0) {
+      unsigned RegEnc = RISCVZC::encodeRlistNumRegs(PushedRegNum);
       MachineInstrBuilder PopBuilder =
           BuildMI(MBB, MI, DL, TII.get(RISCV::CM_POP))
               .setMIFlag(MachineInstr::FrameDestroy);
diff --git a/llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h b/llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h
index 27a13bb7cace1..3f88439086e56 100644
--- a/llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVMachineFunctionInfo.h
@@ -74,7 +74,6 @@ class RISCVMachineFunctionInfo : public MachineFunctionInfo {
   /// Size of stack frame for Zcmp PUSH/POP
   unsigned RVPushStackSize = 0;
   unsigned RVPushRegs = 0;
-  int RVPushRlist = llvm::RISCVZC::RLISTENCODE::INVALID_RLIST;
 
   int64_t StackProbeSize = 0;
 
@@ -146,9 +145,6 @@ class RISCVMachineFunctionInfo : public MachineFunctionInfo {
            VarArgsSaveSize == 0;
   }
 
-  int getRVPushRlist() const { return RVPushRlist; }
-  void setRVPushRlist(int Rlist) { RVPushRlist = Rlist; }
-
   unsigned getRVPushRegs() const { return RVPushRegs; }
   void setRVPushRegs(unsigned Regs) { RVPushRegs = Regs; }
 

Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Two small nits

I don't really like the variable name "PushedRegNum" used in a few places - it reads to me as "Pushed Register Number" (something singular), rather than "Number of Pushed Registers" (something more obviously pushing multiple). I can do that tidy-up later though.

@topperc topperc merged commit 313b71f into llvm:main Mar 3, 2025
9 of 11 checks passed
@topperc topperc deleted the pr/simplify-push-pop-frame branch March 3, 2025 22:38
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
…CVFrameLowering. NFC (llvm#129343)

Previously we calculated the max register id. Then converted it to the number
of registers and encoding. Then converted number of registers to stack
size. Then saved number of registers, encoding, and stack size to
MachineFunctionInfo.

This patch removes the calculation of max register id, and instead
calculates the number of registers. The encoding is removed from
MachineFunctionInfo in favor of converting the number of registers to
encoding at the time of use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants