[BranchRelaxation] Fix invalid branch generation in branch-relaxation

[sc-clulzze]

If we have MBB with only one successor which is accessable through both conditional and unconditional branches (TBB == FBB), in fixupConditionalBranch we will first replace FBB with NewMBB in successors list - MBB->replaceSuccessor(FBB, NewBB);, and then create branch to TBB - insertBranch(MBB, &NextBB, TBB, Cond);, ending up with two branches to different blocks, but only one successor.

Fixes: #162063

[llvmbot]

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

Author: None (sc-clulzze)

Changes

If we have MBB with only one successor which is accessable through both conditional and unconditional branches (TBB == FBB), in fixupConditionalBranch we will first replace FBB with NewMBB in successors list - MBB->replaceSuccessor(FBB, NewBB);, and then create branch to TBB - insertBranch(MBB, &NextBB, TBB, Cond);, ending up with two branches to different blocks, but only one successor.

Fixes: #162063

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/BranchRelaxation.cpp (+14)
• (added) llvm/test/CodeGen/RISCV/branch-rel.mir (+24)

diff --git a/llvm/lib/CodeGen/BranchRelaxation.cpp b/llvm/lib/CodeGen/BranchRelaxation.cpp
index 2d50167faa085..bb75c638d1bcc 100644
--- a/llvm/lib/CodeGen/BranchRelaxation.cpp
+++ b/llvm/lib/CodeGen/BranchRelaxation.cpp
@@ -491,6 +491,20 @@ bool BranchRelaxation::fixupConditionalBranch(MachineInstr &MI) {
       return true;
     }
     if (FBB) {
+      // If we get here with a MBB which ends like this:
+      //
+      // bb.1:
+      // successors: %bb.2;
+      // ...
+      // BNE $x1, $x0, %bb.2
+      // PseudoBR %bb.2
+      //
+      // Just remove conditional branch.
+      if (TBB == FBB) {
+        BlockInfo[MBB->getNumber()].Size -= TII->getInstSizeInBytes(MI);
+        MI.eraseFromParent();
+        return true;
+      }
       // We need to split the basic block here to obtain two long-range
       // unconditional branches.
       NewBB = createNewBlockAfter(*MBB);
diff --git a/llvm/test/CodeGen/RISCV/branch-rel.mir b/llvm/test/CodeGen/RISCV/branch-rel.mir
new file mode 100644
index 0000000000000..218ebe1d09f44
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/branch-rel.mir
@@ -0,0 +1,24 @@
+# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+# RUN: llc %s -mtriple=riscv64 -run-pass=branch-relaxation -o - -verify-machineinstrs | FileCheck %s
+
+--- |
+  define void @foo() {
+    ret void
+  }
+...
+---
+name:            foo
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x1
+    BNE $x1, $x0, %bb.3
+    PseudoBR %bb.3
+  bb.1:
+    liveins: $x1
+    INLINEASM &".space 4096", 1
+    BGE $x1, $x0, %bb.3
+  bb.3:
+    PseudoRET
+## NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+# CHECK: {{.*}}

[sc-clulzze]

@preames, Could you please take a look?

[lenary]

iirc for a MIR test you need update_mir_test_checks.py not the llc version (which expects assembly output) - this is why there are no real CHECK lines in this output.

[sc-clulzze]

Thanks, fixed it, now there are actual CHECKS.

[lenary]

For RISC-V, this seems correct.

I'm not entirely sure if we should be using a sequence of removeBranch(MBB); insertUncondBranch(MBB, TBB) instead of just removing one instruction, not being familiar with how all the targets define removeBranch and insertUnconditionalBranch. From the few I reviewed, they mostly do seem straightforward, but that's not guaranteed?

[sc-clulzze]

I am also not entirely sure about all the targets, but looking at functions description inside TargetInstrInfo.h seems like these functions are indeed pretty straightforward. Lets this sequence instead of explicit eraseFromParent like in other parts of this pass.

[lenary]

LGTM