Reland [InstCombine] Teach foldSelectOpOp about samesign #124320
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:instcombine
|
Inter-diff: diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index d5d9a829c306..635782407704 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -428,16 +428,18 @@ Instruction *InstCombinerImpl::foldSelectOpOp(SelectInst &SI, Instruction *TI,
CmpPredicate TPred, FPred;
if (match(TI, m_ICmp(TPred, m_Value(), m_Value())) &&
match(FI, m_ICmp(FPred, m_Value(), m_Value()))) {
- bool Swapped = ICmpInst::isRelational(FPred) &&
- CmpPredicate::getMatching(
+ auto P = CmpPredicate::getMatching(TPred, FPred);
+ bool Swapped = !P;
+ if (Swapped)
+ P = CmpPredicate::getMatching(
TPred, ICmpInst::getSwappedCmpPredicate(FPred));
- if (CmpPredicate::getMatching(TPred, FPred) || Swapped) {
+ if (P) {
if (Value *MatchOp =
- getCommonOp(TI, FI, ICmpInst::isEquality(TPred), Swapped)) {
+ getCommonOp(TI, FI, ICmpInst::isEquality(*P), Swapped)) {
Value *NewSel = Builder.CreateSelect(Cond, OtherOpT, OtherOpF,
SI.getName() + ".v", &SI);
return new ICmpInst(
- MatchIsOpZero ? TPred : ICmpInst::getSwappedCmpPredicate(TPred),
+ MatchIsOpZero ? *P : ICmpInst::getSwappedCmpPredicate(*P),
MatchOp, NewSel);
}
}
diff --git a/llvm/test/Transforms/InstCombine/select-cmp.ll b/llvm/test/Transforms/InstCombine/select-cmp.ll
index 7e5d5821d9f6..b1bd7a0ecc8a 100644
--- a/llvm/test/Transforms/InstCombine/select-cmp.ll
+++ b/llvm/test/Transforms/InstCombine/select-cmp.ll
@@ -161,7 +161,7 @@ define i1 @icmp_slt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
define i1 @icmp_slt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_slt_samesign_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
-; CHECK-NEXT: [[R:%.*]] = icmp ult i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: [[R:%.*]] = icmp slt i6 [[X:%.*]], [[R_V]]
; CHECK-NEXT: ret i1 [[R]]
;
%cmp1 = icmp samesign ult i6 %x, %y
@@ -185,7 +185,7 @@ define i1 @icmp_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
define i1 @icmp_sgt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_sgt_samesign_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
-; CHECK-NEXT: [[R:%.*]] = icmp ugt i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: [[R:%.*]] = icmp sgt i6 [[X:%.*]], [[R_V]]
; CHECK-NEXT: ret i1 [[R]]
;
%cmp1 = icmp samesign ugt i6 %x, %y
@@ -257,7 +257,7 @@ define i1 @icmp_slt_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
define i1 @icmp_slt_sgt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_slt_sgt_samesign_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
-; CHECK-NEXT: [[R:%.*]] = icmp ult i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: [[R:%.*]] = icmp slt i6 [[X:%.*]], [[R_V]]
; CHECK-NEXT: ret i1 [[R]]
;
%cmp1 = icmp samesign ult i6 %x, %y
@@ -796,5 +796,17 @@ define i1 @sel_icmp_cmp_and_no_simplify_comm(i1 %c, i32 %a1, i32 %a2, i8 %b) {
ret i1 %cmp
}
+define i1 @icmp_lt_slt(i1 %c, i32 %arg) {
+; CHECK-LABEL: @icmp_lt_slt(
+; CHECK-NEXT: [[SELECT_V:%.*]] = select i1 [[C:%.*]], i32 131072, i32 0
+; CHECK-NEXT: [[SELECT:%.*]] = icmp slt i32 [[ARG:%.*]], [[SELECT_V]]
+; CHECK-NEXT: ret i1 [[SELECT]]
+;
+ %cmp1 = icmp samesign ult i32 %arg, 131072
+ %cmp2 = icmp slt i32 %arg, 0
+ %select = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %select
+}
+
declare void @use(i1)
declare void @use.i8(i8) |
|
@llvm/pr-subscribers-llvm-transforms Author: Ramkumar Ramachandra (artagnon) ChangesChanges: There was a serious bug in the previous patch, leading to a miscompile. See #122723 for the miscompile report from Alexander, and the follow-up investigation by Nikita. The patch has since been reworked, and now includes the testcase from the miscompile. Follow up on 4a0d53a (PatternMatch: migrate to CmpPredicate) to get rid of one of the FIXMEs it introduced by replacing a predicate comparison with CmpPredicate::getMatching. Co-authored-by: Nikita Popov <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/124320.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index f66a976ccb47fe..6357824077045f 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -428,16 +428,18 @@ Instruction *InstCombinerImpl::foldSelectOpOp(SelectInst &SI, Instruction *TI,
CmpPredicate TPred, FPred;
if (match(TI, m_ICmp(TPred, m_Value(), m_Value())) &&
match(FI, m_ICmp(FPred, m_Value(), m_Value()))) {
- // FIXME: Use CmpPredicate::getMatching here.
- CmpInst::Predicate T = TPred, F = FPred;
- if (T == F || T == ICmpInst::getSwappedCmpPredicate(F)) {
- bool Swapped = T != F;
+ auto P = CmpPredicate::getMatching(TPred, FPred);
+ bool Swapped = !P;
+ if (Swapped)
+ P = CmpPredicate::getMatching(
+ TPred, ICmpInst::getSwappedCmpPredicate(FPred));
+ if (P) {
if (Value *MatchOp =
- getCommonOp(TI, FI, ICmpInst::isEquality(TPred), Swapped)) {
+ getCommonOp(TI, FI, ICmpInst::isEquality(*P), Swapped)) {
Value *NewSel = Builder.CreateSelect(Cond, OtherOpT, OtherOpF,
SI.getName() + ".v", &SI);
return new ICmpInst(
- MatchIsOpZero ? TPred : ICmpInst::getSwappedCmpPredicate(TPred),
+ MatchIsOpZero ? *P : ICmpInst::getSwappedCmpPredicate(*P),
MatchOp, NewSel);
}
}
diff --git a/llvm/test/Transforms/InstCombine/select-cmp.ll b/llvm/test/Transforms/InstCombine/select-cmp.ll
index f7505bd85f89eb..b1bd7a0ecc8ac6 100644
--- a/llvm/test/Transforms/InstCombine/select-cmp.ll
+++ b/llvm/test/Transforms/InstCombine/select-cmp.ll
@@ -23,6 +23,18 @@ define i1 @icmp_ne_common_op00(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_ne_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_ne_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp ne i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp samesign ne i6 %x, %y
+ %cmp2 = icmp ne i6 %x, %z
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_ne_common_op01(i1 %c, i3 %x, i3 %y, i3 %z) {
; CHECK-LABEL: @icmp_ne_common_op01(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i3 [[Y:%.*]], i3 [[Z:%.*]]
@@ -71,6 +83,18 @@ define i1 @icmp_eq_common_op00(i1 %c, i5 %x, i5 %y, i5 %z) {
ret i1 %r
}
+define i1 @icmp_eq_samesign_common(i1 %c, i5 %x, i5 %y, i5 %z) {
+; CHECK-LABEL: @icmp_eq_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i5 [[Y:%.*]], i5 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp eq i5 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp eq i5 %x, %y
+ %cmp2 = icmp samesign eq i5 %x, %z
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define <5 x i1> @icmp_eq_common_op01(<5 x i1> %c, <5 x i7> %x, <5 x i7> %y, <5 x i7> %z) {
; CHECK-LABEL: @icmp_eq_common_op01(
; CHECK-NEXT: [[R_V:%.*]] = select <5 x i1> [[C:%.*]], <5 x i7> [[Y:%.*]], <5 x i7> [[Z:%.*]]
@@ -134,6 +158,18 @@ define i1 @icmp_slt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_slt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_slt_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp slt i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp samesign ult i6 %x, %y
+ %cmp2 = icmp slt i6 %x, %z
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_sgt_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -146,6 +182,18 @@ define i1 @icmp_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_sgt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_sgt_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp sgt i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp samesign ugt i6 %x, %y
+ %cmp2 = icmp sgt i6 %x, %z
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_sle_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_sle_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -158,6 +206,18 @@ define i1 @icmp_sle_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_sle_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_sle_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp sge i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp sle i6 %y, %x
+ %cmp2 = icmp samesign ule i6 %z, %x
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_sge_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_sge_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -170,6 +230,18 @@ define i1 @icmp_sge_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_sge_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_sge_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp sle i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp sge i6 %y, %x
+ %cmp2 = icmp samesign uge i6 %z, %x
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_slt_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_slt_sgt_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -182,6 +254,18 @@ define i1 @icmp_slt_sgt_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_slt_sgt_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_slt_sgt_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp slt i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp samesign ult i6 %x, %y
+ %cmp2 = icmp sgt i6 %z, %x
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_sle_sge_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_sle_sge_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -194,6 +278,18 @@ define i1 @icmp_sle_sge_common(i1 %c, i6 %x, i6 %y, i6 %z) {
ret i1 %r
}
+define i1 @icmp_sle_sge_samesign_common(i1 %c, i6 %x, i6 %y, i6 %z) {
+; CHECK-LABEL: @icmp_sle_sge_samesign_common(
+; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
+; CHECK-NEXT: [[R:%.*]] = icmp sge i6 [[X:%.*]], [[R_V]]
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %cmp1 = icmp sle i6 %y, %x
+ %cmp2 = icmp samesign uge i6 %x, %z
+ %r = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %r
+}
+
define i1 @icmp_ult_common(i1 %c, i6 %x, i6 %y, i6 %z) {
; CHECK-LABEL: @icmp_ult_common(
; CHECK-NEXT: [[R_V:%.*]] = select i1 [[C:%.*]], i6 [[Y:%.*]], i6 [[Z:%.*]]
@@ -700,5 +796,17 @@ define i1 @sel_icmp_cmp_and_no_simplify_comm(i1 %c, i32 %a1, i32 %a2, i8 %b) {
ret i1 %cmp
}
+define i1 @icmp_lt_slt(i1 %c, i32 %arg) {
+; CHECK-LABEL: @icmp_lt_slt(
+; CHECK-NEXT: [[SELECT_V:%.*]] = select i1 [[C:%.*]], i32 131072, i32 0
+; CHECK-NEXT: [[SELECT:%.*]] = icmp slt i32 [[ARG:%.*]], [[SELECT_V]]
+; CHECK-NEXT: ret i1 [[SELECT]]
+;
+ %cmp1 = icmp samesign ult i32 %arg, 131072
+ %cmp2 = icmp slt i32 %arg, 0
+ %select = select i1 %c, i1 %cmp1, i1 %cmp2
+ ret i1 %select
+}
+
declare void @use(i1)
declare void @use.i8(i8)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
artagnon
force-pushed
the
ic-samesign-getmatching-reland
branch
from
4784330 to
6e8034b
Compare
nikic
reviewed
Jan 25, 2025
goldsteinn
reviewed
Jan 27, 2025
Changes: There was a serious bug in the previous patch, leading to a miscompile. See llvm#122723 for the miscompile report from Alexander, and the follow-up investigation by Nikita. The patch has since been reworked, and now includes the testcase from the miscompile. Follow up on 4a0d53a (PatternMatch: migrate to CmpPredicate) to get rid of one of the FIXMEs it introduced by replacing a predicate comparison with CmpPredicate::getMatching. Co-authored-by: Nikita Popov <[email protected]>
artagnon
force-pushed
the
ic-samesign-getmatching-reland
branch
from
0a37849 to
d0bdc33
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes: There was a serious bug in the previous patch, leading to a miscompile. See #122723 for the miscompile report from Alexander, and the follow-up investigation by Nikita. The patch has since been reworked, and now includes the testcase from the miscompile.
Follow up on 4a0d53a (PatternMatch: migrate to CmpPredicate) to get rid of one of the FIXMEs it introduced by replacing a predicate comparison with CmpPredicate::getMatching.
Co-authored-by: Nikita Popov [email protected]