[RISCV] fix RISCVPushPopOptimizer pass

[dlav-sc]

RISCVPushPopOptimizer pass didn't suggest that CFI instructions can be inserted between cm.pop and ret and couldn't apply optimization in these cases. The patch fix it and allows the pass to remove CFI instructions and combine cm.pop and ret into the one instruction: cm.popret or cm.popretz.

[llvmbot]

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

Author: None (dlav-sc)

Changes

RISCVPushPopOptimizer pass didn't suggest that CFI instructions can be inserted between cm.pop and ret and couldn't apply optimization in these cases. The patch fix it and allows the pass to remove CFI instructions and combine cm.pop and ret into the one instruction: cm.popret or cm.popretz.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp (+28-4)

diff --git a/llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
index 098e5bb5328bb3..c05c56a758b558 100644
--- a/llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
+++ b/llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
@@ -45,10 +45,30 @@ char RISCVPushPopOpt::ID = 0;
 INITIALIZE_PASS(RISCVPushPopOpt, "riscv-push-pop-opt", RISCV_PUSH_POP_OPT_NAME,
                 false, false)
 
+template <typename IterT>
+static IterT nextNoDebugNoCFIInst(const IterT It, const IterT End) {
+  assert(It != End);
+  return std::find_if_not(std::next(It), End, [](const auto &Inst) {
+    return Inst.isDebugInstr() || Inst.isCFIInstruction() ||
+           Inst.isPseudoProbe();
+  });
+}
+
+static void eraseCFIInst(const MachineBasicBlock::iterator Begin,
+                         const MachineBasicBlock::iterator End) {
+  assert(Begin != End);
+  std::vector<std::reference_wrapper<llvm::MachineInstr>> CFAInstrs;
+  std::copy_if(std::next(Begin), End, std::back_inserter(CFAInstrs),
+               [](const auto &Inst) { return Inst.isCFIInstruction(); });
+
+  for (auto Inst : CFAInstrs)
+    Inst.get().eraseFromParent();
+}
+
 // Check if POP instruction was inserted into the MBB and return iterator to it.
 static MachineBasicBlock::iterator containsPop(MachineBasicBlock &MBB) {
   for (MachineBasicBlock::iterator MBBI = MBB.begin(); MBBI != MBB.end();
-       MBBI = next_nodbg(MBBI, MBB.end()))
+       MBBI = nextNoDebugNoCFIInst(MBBI, MBB.end()))
     if (MBBI->getOpcode() == RISCV::CM_POP)
       return MBBI;
 
@@ -76,6 +96,10 @@ bool RISCVPushPopOpt::usePopRet(MachineBasicBlock::iterator &MBBI,
   for (unsigned i = FirstNonDeclaredOp; i < MBBI->getNumOperands(); ++i)
     PopRetBuilder.add(MBBI->getOperand(i));
 
+  // Remove CFI instructions, they are not needed for cm.popret and cm.popretz
+  // anyway
+  eraseCFIInst(MBBI, NextI);
+
   MBBI->eraseFromParent();
   NextI->eraseFromParent();
   return true;
@@ -92,8 +116,8 @@ bool RISCVPushPopOpt::adjustRetVal(MachineBasicBlock::iterator &MBBI) {
   // Since POP instruction is in Epilogue no normal instructions will follow
   // after it. Therefore search only previous ones to find the return value.
   for (MachineBasicBlock::reverse_iterator I =
-           next_nodbg(MBBI.getReverse(), RE);
-       I != RE; I = next_nodbg(I, RE)) {
+           nextNoDebugNoCFIInst(MBBI.getReverse(), RE);
+       I != RE; I = nextNoDebugNoCFIInst(I, RE)) {
     MachineInstr &MI = *I;
     if (auto OperandPair = TII->isCopyInstrImpl(MI)) {
       Register DestReg = OperandPair->Destination->getReg();
@@ -138,7 +162,7 @@ bool RISCVPushPopOpt::runOnMachineFunction(MachineFunction &Fn) {
   bool Modified = false;
   for (auto &MBB : Fn) {
     MachineBasicBlock::iterator MBBI = containsPop(MBB);
-    MachineBasicBlock::iterator NextI = next_nodbg(MBBI, MBB.end());
+    MachineBasicBlock::iterator NextI = nextNoDebugNoCFIInst(MBBI, MBB.end());
     if (MBBI != MBB.end() && NextI != MBB.end() &&
         NextI->getOpcode() == RISCV::PseudoRET)
       Modified |= usePopRet(MBBI, NextI, adjustRetVal(MBBI));

[dlav-sc]

This patch is necessary for #110234

[topperc]

Why do we need to copy to a vector?

[dlav-sc]

addressed

[github-actions]

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

[michaelmaitland]

test case?

[dlav-sc]

test case?

This patch supports #110234, it doesn't really matter alone. I'm going to split #110234 into several PRs and after that this one will be a part of stacked PRs I guess.

[dlav-sc]

Actual version: #110813

[michaelmaitland]

Actual version: #110813

Is this patch a duplicate? If so, we should close one of them.

[dlav-sc]

we should close one of them.

yep