Skip to content

[ValueTracking] Handle non-canonical operand order in isImpliedCondICmps #85575

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

Closed
wants to merge 2 commits into from
Closed

[ValueTracking] Handle non-canonical operand order in isImpliedCondICmps #85575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d144e08
[ValueTracking] Add tests for implied cond with swapped operands; NFC
goldsteinn Mar 17, 2024
beb6e1a
[ValueTracking] Handle non-canonical operand order in `isImpliedCondI…
goldsteinn Mar 17, 2024
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
66 changes: 27 additions & 39 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8471,26 +8471,12 @@ isImpliedCondOperands(CmpInst::Predicate Pred, const Value *ALHS,
}
}

/// Return true if the operands of two compares (expanded as "L0 pred L1" and
/// "R0 pred R1") match. IsSwappedOps is true when the operands match, but are
/// swapped.
static bool areMatchingOperands(const Value *L0, const Value *L1, const Value *R0,
const Value *R1, bool &AreSwappedOps) {
bool AreMatchingOps = (L0 == R0 && L1 == R1);
AreSwappedOps = (L0 == R1 && L1 == R0);
return AreMatchingOps || AreSwappedOps;
}

/// Return true if "icmp1 LPred X, Y" implies "icmp2 RPred X, Y" is true.
/// Return false if "icmp1 LPred X, Y" implies "icmp2 RPred X, Y" is false.
/// Otherwise, return std::nullopt if we can't infer anything.
static std::optional<bool>
isImpliedCondMatchingOperands(CmpInst::Predicate LPred,
CmpInst::Predicate RPred, bool AreSwappedOps) {
// Canonicalize the predicate as if the operands were not commuted.
if (AreSwappedOps)
RPred = ICmpInst::getSwappedPredicate(RPred);

CmpInst::Predicate RPred) {
if (CmpInst::isImpliedTrueByMatchingCmp(LPred, RPred))
return true;
if (CmpInst::isImpliedFalseByMatchingCmp(LPred, RPred))
Expand Down Expand Up @@ -8532,39 +8518,41 @@ static std::optional<bool> isImpliedCondICmps(const ICmpInst *LHS,
CmpInst::Predicate LPred =
LHSIsTrue ? LHS->getPredicate() : LHS->getInversePredicate();

// We can have non-canonical operands, so try to normalize any common operand
// to L0/R0.
if (L0 == R1) {
std::swap(R0, R1);
RPred = ICmpInst::getSwappedPredicate(RPred);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we handle this by checking operand equality instead, as you did in the other patch? I'm thinking there is some cleanup to be done here: We have areMatchingOperands checking for swap, and then we also have it in the isUnsigned() code path below, and then this adds another case.

Or does that run afoul of the case where the constants are equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking matching does work (didn't see any difference between the two). Ill update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop the swapping code below now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup with a little bit more work in the initial swaps.

if (R0 == L1) {
std::swap(L0, L1);
LPred = ICmpInst::getSwappedPredicate(LPred);
}
if (L1 == R1) {
// If we have L0 == R0 and L1 == R1, then make L1/R1 the constants.
if (L0 != R0 || match(L0, m_ImmConstant())) {
std::swap(L0, L1);
LPred = ICmpInst::getSwappedPredicate(LPred);
std::swap(R0, R1);
RPred = ICmpInst::getSwappedPredicate(RPred);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Add a newline here?

// Can we infer anything when the 0-operands match and the 1-operands are
// constants (not necessarily matching)?
const APInt *LC, *RC;
if (L0 == R0 && match(L1, m_APInt(LC)) && match(R1, m_APInt(RC)))
return isImpliedCondCommonOperandWithConstants(LPred, *LC, RPred, *RC);

// Can we infer anything when the two compares have matching operands?
bool AreSwappedOps;
if (areMatchingOperands(L0, L1, R0, R1, AreSwappedOps))
return isImpliedCondMatchingOperands(LPred, RPred, AreSwappedOps);
if (L0 == R0 && L1 == R1)
return isImpliedCondMatchingOperands(LPred, RPred);

// L0 = R0 = L1 + R1, L0 >=u L1 implies R0 >=u R1, L0 <u L1 implies R0 <u R1
if (ICmpInst::isUnsigned(LPred) && ICmpInst::isUnsigned(RPred)) {
if (L0 == R1) {
std::swap(R0, R1);
RPred = ICmpInst::getSwappedPredicate(RPred);
}
if (L1 == R0) {
std::swap(L0, L1);
LPred = ICmpInst::getSwappedPredicate(LPred);
}
if (L1 == R1) {
std::swap(L0, L1);
LPred = ICmpInst::getSwappedPredicate(LPred);
std::swap(R0, R1);
RPred = ICmpInst::getSwappedPredicate(RPred);
}
if (L0 == R0 &&
(LPred == ICmpInst::ICMP_ULT || LPred == ICmpInst::ICMP_UGE) &&
(RPred == ICmpInst::ICMP_ULT || RPred == ICmpInst::ICMP_UGE) &&
match(L0, m_c_Add(m_Specific(L1), m_Specific(R1))))
return LPred == RPred;
}
if (L0 == R0 &&
(LPred == ICmpInst::ICMP_ULT || LPred == ICmpInst::ICMP_UGE) &&
(RPred == ICmpInst::ICMP_ULT || RPred == ICmpInst::ICMP_UGE) &&
match(L0, m_c_Add(m_Specific(L1), m_Specific(R1))))
return LPred == RPred;

if (LPred == RPred)
return isImpliedCondOperands(LPred, L0, L1, R0, R1, DL, Depth);
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/InstCombine/assume.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ define i1 @nonnull5(ptr %a) {
define i32 @assumption_conflicts_with_known_bits(i32 %a, i32 %b) {
; CHECK-LABEL: @assumption_conflicts_with_known_bits(
; CHECK-NEXT: store i1 true, ptr poison, align 1
; CHECK-NEXT: ret i32 1
; CHECK-NEXT: ret i32 poison
;
%and1 = and i32 %b, 3
%B1 = lshr i32 %and1, %and1
Expand Down
93 changes: 93 additions & 0 deletions llvm/test/Transforms/InstCombine/icmp-select-implies-common-op.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=instcombine < %s | FileCheck %s

define i1 @sgt_3_impliesF_eq_2(i8 %x, i8 %y) {
; CHECK-LABEL: @sgt_3_impliesF_eq_2(
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 [[X:%.*]], 4
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i8 [[SEL:%.*]], [[X]]
; CHECK-NEXT: [[CMP3:%.*]] = select i1 [[CMP]], i1 [[CMP2]], i1 false
; CHECK-NEXT: ret i1 [[CMP3]]
;
%cmp = icmp sgt i8 %x, 3
%sel = select i1 %cmp, i8 2, i8 %y
%cmp2 = icmp eq i8 %sel, %x
ret i1 %cmp2
}

define i1 @sgt_3_impliesT_sgt_2(i8 %x, i8 %y) {
; CHECK-LABEL: @sgt_3_impliesT_sgt_2(
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 [[X:%.*]], 4
; CHECK-NEXT: [[CMP2:%.*]] = icmp sgt i8 [[SEL:%.*]], [[X]]
; CHECK-NEXT: [[CMP3:%.*]] = select i1 [[CMP]], i1 [[CMP2]], i1 false
; CHECK-NEXT: ret i1 [[CMP3]]
;
%cmp = icmp sgt i8 %x, 3
%sel = select i1 %cmp, i8 2, i8 %y
%cmp2 = icmp sgt i8 %sel, %x
ret i1 %cmp2
}

define i1 @sgt_x_impliesF_eq_smin_todo(i8 %x, i8 %y, i8 %z) {
; CHECK-LABEL: @sgt_x_impliesF_eq_smin_todo(
; CHECK-NEXT: [[CMP:%.*]] = icmp sgt i8 [[X:%.*]], [[Z:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i8 -128, i8 [[Y:%.*]]
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i8 [[SEL]], [[X]]
; CHECK-NEXT: ret i1 [[CMP2]]
;
%cmp = icmp sgt i8 %x, %z
%sel = select i1 %cmp, i8 -128, i8 %y
%cmp2 = icmp eq i8 %sel, %x
ret i1 %cmp2
}

define i1 @slt_x_impliesT_ne_smin_todo(i8 %x, i8 %y, i8 %z) {
; CHECK-LABEL: @slt_x_impliesT_ne_smin_todo(
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 [[X:%.*]], [[Z:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i8 127, i8 [[Y:%.*]]
; CHECK-NEXT: [[CMP2:%.*]] = icmp ne i8 [[SEL]], [[X]]
; CHECK-NEXT: ret i1 [[CMP2]]
;
%cmp = icmp slt i8 %x, %z
%sel = select i1 %cmp, i8 127, i8 %y
%cmp2 = icmp ne i8 %x, %sel
ret i1 %cmp2
}

define i1 @ult_x_impliesT_eq_umax_todo(i8 %x, i8 %y, i8 %z) {
; CHECK-LABEL: @ult_x_impliesT_eq_umax_todo(
; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[Z:%.*]], [[X:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i8 -1, i8 [[Y:%.*]]
; CHECK-NEXT: [[CMP2:%.*]] = icmp ne i8 [[SEL]], [[X]]
; CHECK-NEXT: ret i1 [[CMP2]]
;
%cmp = icmp ugt i8 %z, %x
%sel = select i1 %cmp, i8 255, i8 %y
%cmp2 = icmp ne i8 %sel, %x
ret i1 %cmp2
}

define i1 @ult_1_impliesF_eq_1(i8 %x, i8 %y) {
; CHECK-LABEL: @ult_1_impliesF_eq_1(
; CHECK-NEXT: [[CMP:%.*]] = icmp ne i8 [[SEL:%.*]], 0
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i8 [[X:%.*]], [[SEL]]
; CHECK-NEXT: [[CMP3:%.*]] = select i1 [[CMP]], i1 [[CMP2]], i1 false
; CHECK-NEXT: ret i1 [[CMP3]]
;
%cmp = icmp ult i8 %x, 1
%sel = select i1 %cmp, i8 1, i8 %y
%cmp2 = icmp eq i8 %x, %sel
ret i1 %cmp2
}

define i1 @ugt_x_impliesF_eq_umin_todo(i8 %x, i8 %y, i8 %z) {
; CHECK-LABEL: @ugt_x_impliesF_eq_umin_todo(
; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[Z:%.*]], [[X:%.*]]
; CHECK-NEXT: [[SEL:%.*]] = select i1 [[CMP]], i8 0, i8 [[Y:%.*]]
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i8 [[SEL]], [[X]]
; CHECK-NEXT: ret i1 [[CMP2]]
;
%cmp = icmp ugt i8 %z, %x
%sel = select i1 %cmp, i8 0, i8 %y
%cmp2 = icmp eq i8 %x, %sel
ret i1 %cmp2
}
6 changes: 2 additions & 4 deletions llvm/test/Transforms/InstCombine/select.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2925,10 +2925,8 @@ define i8 @select_replacement_loop3(i32 noundef %x) {

define i16 @select_replacement_loop4(i16 noundef %p_12) {
; CHECK-LABEL: @select_replacement_loop4(
; CHECK-NEXT: [[CMP1:%.*]] = icmp ult i16 [[P_12:%.*]], 2
; CHECK-NEXT: [[AND1:%.*]] = and i16 [[P_12]], 1
; CHECK-NEXT: [[AND2:%.*]] = select i1 [[CMP1]], i16 [[AND1]], i16 0
; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i16 [[AND2]], [[P_12]]
; CHECK-NEXT: [[AND1:%.*]] = and i16 [[P_12:%.*]], 1
; CHECK-NEXT: [[CMP2:%.*]] = icmp ult i16 [[P_12]], 2
; CHECK-NEXT: [[AND3:%.*]] = select i1 [[CMP2]], i16 [[AND1]], i16 0
; CHECK-NEXT: ret i16 [[AND3]]
;
Expand Down
7 changes: 3 additions & 4 deletions llvm/test/Transforms/InstCombine/shift.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1751,12 +1751,11 @@ define void @ashr_out_of_range_1(ptr %A) {
; CHECK-NEXT: [[L:%.*]] = load i177, ptr [[A:%.*]], align 4
; CHECK-NEXT: [[L_FROZEN:%.*]] = freeze i177 [[L]]
; CHECK-NEXT: [[TMP1:%.*]] = icmp eq i177 [[L_FROZEN]], -1
; CHECK-NEXT: [[B:%.*]] = select i1 [[TMP1]], i177 0, i177 [[L_FROZEN]]
; CHECK-NEXT: [[TMP2:%.*]] = trunc i177 [[B]] to i64
; CHECK-NEXT: [[TMP6:%.*]] = trunc i177 [[L_FROZEN]] to i64
; CHECK-NEXT: [[TMP2:%.*]] = select i1 [[TMP1]], i64 0, i64 [[TMP6]]
; CHECK-NEXT: [[TMP3:%.*]] = getelementptr i177, ptr [[A]], i64 [[TMP2]]
; CHECK-NEXT: [[G11:%.*]] = getelementptr i8, ptr [[TMP3]], i64 -24
; CHECK-NEXT: [[C17:%.*]] = icmp sgt i177 [[B]], [[L_FROZEN]]
; CHECK-NEXT: [[TMP4:%.*]] = sext i1 [[C17]] to i64
; CHECK-NEXT: [[TMP4:%.*]] = sext i1 [[TMP1]] to i64
; CHECK-NEXT: [[G62:%.*]] = getelementptr i177, ptr [[G11]], i64 [[TMP4]]
; CHECK-NEXT: [[TMP5:%.*]] = icmp eq i177 [[L_FROZEN]], -1
; CHECK-NEXT: [[B28:%.*]] = select i1 [[TMP5]], i177 0, i177 [[L_FROZEN]]
Expand Down