Skip to content

Conversation

@daniilavdeev
Copy link
Contributor

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
Copy link
Member

llvmbot commented Sep 27, 2024

@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));

@daniilavdeev
Copy link
Contributor Author

This patch is necessary for #110234

@daniilavdeev daniilavdeev force-pushed the dlav-sc/riscv-push-pop-fix branch 2 times, most recently from 2cd6339 to 7299365 Compare September 27, 2024 12:02

static void eraseCFIInst(const MachineBasicBlock::iterator Begin,
const MachineBasicBlock::iterator End) {
std::vector<std::reference_wrapper<llvm::MachineInstr>> CFIInstrs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to copy to a vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@daniilavdeev daniilavdeev force-pushed the dlav-sc/riscv-push-pop-fix branch from 7299365 to c76c3e8 Compare October 1, 2024 10:37
@github-actions
Copy link

github-actions bot commented Oct 1, 2024

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

@daniilavdeev daniilavdeev force-pushed the dlav-sc/riscv-push-pop-fix branch 2 times, most recently from cfcb8a2 to e03a5fc Compare October 1, 2024 10:55
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.
Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

test case?

@daniilavdeev
Copy link
Contributor Author

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.

@daniilavdeev
Copy link
Contributor Author

Actual version: #110813

@michaelmaitland
Copy link
Contributor

Actual version: #110813

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

@daniilavdeev
Copy link
Contributor Author

we should close one of them.

yep

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.

4 participants