Skip to content

[RISCV] Fix a correctness issue in optimizeCondBranch. Prevent optimizing compare with x0. NFC #145440

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
topperc merged 4 commits into from
Jun 24, 2025
Merged

[RISCV] Fix a correctness issue in optimizeCondBranch. Prevent optimizing compare with x0. NFC #145440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edf44b5
[RISCV] Add more tests for optimizeCondBranch. NFC
topperc Jun 23, 2025
26cba07
[RISCV] Fix a correctness issue in optimizeCondBranch. Prevent optimi…
topperc Jun 23, 2025
b1b61f6
fixup! address review comments
topperc Jun 24, 2025
32dd880
fixup! Add comment for the incorrect signed cases.
topperc Jun 24, 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
69 changes: 39 additions & 30 deletions llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,36 +1449,45 @@ bool RISCVInstrInfo::optimizeCondBranch(MachineInstr &MI) const {
return Register();
};

if (isFromLoadImm(MRI, LHS, C0) && MRI.hasOneUse(LHS.getReg())) {
// Might be case 1.
// Signed integer overflow is UB. (UINT64_MAX is bigger so we don't need
// to worry about unsigned overflow here)
if (C0 < INT64_MAX)
if (Register RegZ = searchConst(C0 + 1)) {
reverseBranchCondition(Cond);
Cond[1] = MachineOperand::CreateReg(RHS.getReg(), /*isDef=*/false);
Cond[2] = MachineOperand::CreateReg(RegZ, /*isDef=*/false);
// We might extend the live range of Z, clear its kill flag to
// account for this.
MRI.clearKillFlags(RegZ);
modifyBranch();
return true;
}
} else if (isFromLoadImm(MRI, RHS, C0) && MRI.hasOneUse(RHS.getReg())) {
// Might be case 2.
// For unsigned cases, we don't want C1 to wrap back to UINT64_MAX
// when C0 is zero.
if ((CC == RISCVCC::COND_GE || CC == RISCVCC::COND_LT) || C0)
if (Register RegZ = searchConst(C0 - 1)) {
reverseBranchCondition(Cond);
Cond[1] = MachineOperand::CreateReg(RegZ, /*isDef=*/false);
Cond[2] = MachineOperand::CreateReg(LHS.getReg(), /*isDef=*/false);
// We might extend the live range of Z, clear its kill flag to
// account for this.
MRI.clearKillFlags(RegZ);
modifyBranch();
return true;
}
// Might be case 1.
// Don't change 0 to 1 since we can use x0.
// For unsigned cases changing -1U to 0 would be incorrect.
// The incorrect case for signed would be INT_MAX, but isFromLoadImm can't
// return that.
if (isFromLoadImm(MRI, LHS, C0) && C0 != 0 && LHS.getReg().isVirtual() &&
MRI.hasOneUse(LHS.getReg()) &&
(CC == RISCVCC::COND_GE || CC == RISCVCC::COND_LT || C0 != -1)) {
assert(isInt<12>(C0) && "Unexpected immediate");
Copy link
Member

@mshockwave mshockwave Jun 24, 2025

Choose a reason for hiding this comment

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

I was having the same question with Philip as where did we prevent sign overflow from happening, so perhaps we can leave a comment here?

if (Register RegZ = searchConst(C0 + 1)) {
reverseBranchCondition(Cond);
Cond[1] = MachineOperand::CreateReg(RHS.getReg(), /*isDef=*/false);
Cond[2] = MachineOperand::CreateReg(RegZ, /*isDef=*/false);
// We might extend the live range of Z, clear its kill flag to
// account for this.
MRI.clearKillFlags(RegZ);
modifyBranch();
return true;
}
}

// Might be case 2.
// For signed cases we don't want to change 0 since we can use x0.
// For unsigned cases changing 0 to -1U would be incorrect.
// The incorrect case for signed would be INT_MIN, but isFromLoadImm can't
// return that.
if (isFromLoadImm(MRI, RHS, C0) && C0 != 0 && RHS.getReg().isVirtual() &&
MRI.hasOneUse(RHS.getReg())) {
assert(isInt<12>(C0) && "Unexpected immediate");
if (Register RegZ = searchConst(C0 - 1)) {
reverseBranchCondition(Cond);
Cond[1] = MachineOperand::CreateReg(RegZ, /*isDef=*/false);
Cond[2] = MachineOperand::CreateReg(LHS.getReg(), /*isDef=*/false);
// We might extend the live range of Z, clear its kill flag to
// account for this.
MRI.clearKillFlags(RegZ);
modifyBranch();
return true;
}
}

return false;
Expand Down
308 changes: 308 additions & 0 deletions llvm/test/CodeGen/RISCV/branch-opt.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,74 @@
ret void
}

define void @ule_negone(ptr %a, i32 signext %b, ptr %c, ptr %d) {
store i32 0, ptr %a
%p = icmp ule i32 %b, -1
br i1 %p, label %block1, label %block2

block1: ; preds = %0
store i32 %b, ptr %c
br label %end_block

block2: ; preds = %0
store i32 87, ptr %d
br label %end_block

end_block: ; preds = %block2, %block1
ret void
}

define void @ult_zero(ptr %a, i32 signext %b, ptr %c, ptr %d) {
store i32 -1, ptr %a
%p = icmp ult i32 %b, 0
br i1 %p, label %block1, label %block2

block1: ; preds = %0
store i32 %b, ptr %c
br label %end_block

block2: ; preds = %0
store i32 87, ptr %d
br label %end_block

end_block: ; preds = %block2, %block1
ret void
}

define void @sle_zero(ptr %a, i32 signext %b, ptr %c, ptr %d) {
store i32 1, ptr %a
%p = icmp sle i32 %b, 0
br i1 %p, label %block1, label %block2

block1: ; preds = %0
store i32 %b, ptr %c
br label %end_block

block2: ; preds = %0
store i32 87, ptr %d
br label %end_block

end_block: ; preds = %block2, %block1
ret void
}

define void @slt_zero(ptr %a, i32 signext %b, ptr %c, ptr %d) {
store i32 -1, ptr %a
%p = icmp slt i32 %b, 0
br i1 %p, label %block1, label %block2

block1: ; preds = %0
store i32 %b, ptr %c
br label %end_block

block2: ; preds = %0
store i32 87, ptr %d
br label %end_block

end_block: ; preds = %block2, %block1
ret void
}

declare void @bar(...)

...
Expand Down Expand Up @@ -66,3 +134,243 @@ body: |
PseudoRET

...
---
name: ule_negone
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: ule_negone
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x10, $x11, $x12, $x13
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x13
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x12
; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI $x0, 0
; CHECK-NEXT: SW killed [[ADDI]], [[COPY3]], 0 :: (store (s32))
; CHECK-NEXT: [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, -1
; CHECK-NEXT: BLTU killed [[ADDI1]], [[COPY2]], %bb.2
; CHECK-NEXT: PseudoBR %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr = COPY [[COPY2]]
; CHECK-NEXT: SW [[COPY4]], [[COPY1]], 0 :: (store (s32))
; CHECK-NEXT: PseudoBR %bb.3
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 87
; CHECK-NEXT: SW killed [[ADDI2]], [[COPY]], 0 :: (store (s32))
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.3:
; CHECK-NEXT: PseudoRET
bb.0:
successors: %bb.1, %bb.2
liveins: $x10, $x11, $x12, $x13

%3:gpr = COPY $x13
%2:gpr = COPY $x12
%1:gpr = COPY $x11
%0:gpr = COPY $x10
%5:gpr = ADDI $x0, 0
SW killed %5, %0, 0 :: (store (s32))
%6:gpr = ADDI $x0, -1
BLTU killed %6, %1, %bb.2
PseudoBR %bb.1

bb.1:
%4:gpr = COPY %1
SW %4, %2, 0 :: (store (s32))
PseudoBR %bb.3

bb.2:
%7:gpr = ADDI $x0, 87
SW killed %7, %3, 0 :: (store (s32))

bb.3:
PseudoRET
...
---
name: ult_zero
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: ult_zero
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x10, $x11, $x12, $x13
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x13
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x12
; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI $x0, -1
; CHECK-NEXT: SW killed [[ADDI]], [[COPY3]], 0 :: (store (s32))
; CHECK-NEXT: [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, 0
; CHECK-NEXT: BLTU [[COPY2]], killed [[ADDI1]], %bb.2
; CHECK-NEXT: PseudoBR %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr = COPY [[COPY2]]
; CHECK-NEXT: SW [[COPY4]], [[COPY1]], 0 :: (store (s32))
; CHECK-NEXT: PseudoBR %bb.3
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 87
; CHECK-NEXT: SW killed [[ADDI2]], [[COPY]], 0 :: (store (s32))
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.3:
; CHECK-NEXT: PseudoRET
bb.0:
successors: %bb.1, %bb.2
liveins: $x10, $x11, $x12, $x13

%3:gpr = COPY $x13
%2:gpr = COPY $x12
%1:gpr = COPY $x11
%0:gpr = COPY $x10
%5:gpr = ADDI $x0, -1
SW killed %5, %0, 0 :: (store (s32))
%6:gpr = ADDI $x0, 0
BLTU %1, killed %6, %bb.2
PseudoBR %bb.1

bb.1:
%4:gpr = COPY %1
SW %4, %2, 0 :: (store (s32))
PseudoBR %bb.3

bb.2:
%7:gpr = ADDI $x0, 87
SW killed %7, %3, 0 :: (store (s32))

bb.3:
PseudoRET
...
---
name: sle_zero
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: sle_zero
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x10, $x11, $x12, $x13
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x13
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x12
; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI $x0, 1
; CHECK-NEXT: SW killed [[ADDI]], [[COPY3]], 0 :: (store (s32))
; CHECK-NEXT: [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, 0
; CHECK-NEXT: BLT killed [[ADDI1]], [[COPY2]], %bb.2
; CHECK-NEXT: PseudoBR %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr = COPY [[COPY2]]
; CHECK-NEXT: SW [[COPY4]], [[COPY1]], 0 :: (store (s32))
; CHECK-NEXT: PseudoBR %bb.3
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 87
; CHECK-NEXT: SW killed [[ADDI2]], [[COPY]], 0 :: (store (s32))
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.3:
; CHECK-NEXT: PseudoRET
bb.0:
successors: %bb.1, %bb.2
liveins: $x10, $x11, $x12, $x13

%3:gpr = COPY $x13
%2:gpr = COPY $x12
%1:gpr = COPY $x11
%0:gpr = COPY $x10
%5:gpr = ADDI $x0, 1
SW killed %5, %0, 0 :: (store (s32))
%6:gpr = ADDI $x0, 0
BLT killed %6, %1, %bb.2
PseudoBR %bb.1

bb.1:
%4:gpr = COPY %1
SW %4, %2, 0 :: (store (s32))
PseudoBR %bb.3

bb.2:
%7:gpr = ADDI $x0, 87
SW killed %7, %3, 0 :: (store (s32))

bb.3:
PseudoRET
...
---
name: slt_zero
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: slt_zero
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK-NEXT: liveins: $x10, $x11, $x12, $x13
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x13
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x12
; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[ADDI:%[0-9]+]]:gpr = ADDI $x0, -1
; CHECK-NEXT: SW killed [[ADDI]], [[COPY3]], 0 :: (store (s32))
; CHECK-NEXT: [[ADDI1:%[0-9]+]]:gpr = ADDI $x0, 0
; CHECK-NEXT: BLT [[COPY2]], killed [[ADDI1]], %bb.2
; CHECK-NEXT: PseudoBR %bb.1
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr = COPY [[COPY2]]
; CHECK-NEXT: SW [[COPY4]], [[COPY1]], 0 :: (store (s32))
; CHECK-NEXT: PseudoBR %bb.3
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: successors: %bb.3(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[ADDI2:%[0-9]+]]:gpr = ADDI $x0, 87
; CHECK-NEXT: SW killed [[ADDI2]], [[COPY]], 0 :: (store (s32))
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.3:
; CHECK-NEXT: PseudoRET
bb.0:
successors: %bb.1, %bb.2
liveins: $x10, $x11, $x12, $x13

%3:gpr = COPY $x13
%2:gpr = COPY $x12
%1:gpr = COPY $x11
%0:gpr = COPY $x10
%5:gpr = ADDI $x0, -1
SW killed %5, %0, 0 :: (store (s32))
%6:gpr = ADDI $x0, 0
BLT %1, killed %6, %bb.2
PseudoBR %bb.1

bb.1:
%4:gpr = COPY %1
SW %4, %2, 0 :: (store (s32))
PseudoBR %bb.3

bb.2:
%7:gpr = ADDI $x0, 87
SW killed %7, %3, 0 :: (store (s32))

bb.3:
PseudoRET
...
Loading