Skip to content

[BranchRelaxation] Fix invalid branch generation in branch-relaxation #162065

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.

Sign up for GitHub

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
Oct 15, 2025
Merged

[BranchRelaxation] Fix invalid branch generation in branch-relaxation #162065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33b74f5
[BranchRelaxation] Fix invalid branch generation in branch-relaxation
sc-clulzze Oct 3, 2025
fd7036e
Generate test checks correctly
sc-clulzze Oct 6, 2025
3694e61
Use remove/insert branch instead of direct eraseFromParent
sc-clulzze Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions llvm/lib/CodeGen/BranchRelaxation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines 503 to 507
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@sc-clulzze sc-clulzze Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// We need to split the basic block here to obtain two long-range
// unconditional branches.
NewBB = createNewBlockAfter(*MBB);
Expand Down
24 changes: 24 additions & 0 deletions llvm/test/CodeGen/RISCV/branch-rel.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed it, now there are actual CHECKS.

# 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: {{.*}}