[InstCombine] Teach foldSelectOpOp about samesign #122723
Merged
+100
−4
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
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.
llvmbot
added
llvm:instcombine
|
@llvm/pr-subscribers-llvm-transforms Author: Ramkumar Ramachandra (artagnon) ChangesFollow 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. Full diff: https://github.com/llvm/llvm-project/pull/122723.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index f66a976ccb47fe..d5d9a829c3068a 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -428,10 +428,10 @@ 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;
+ bool Swapped = ICmpInst::isRelational(FPred) &&
+ CmpPredicate::getMatching(
+ TPred, ICmpInst::getSwappedCmpPredicate(FPred));
+ if (CmpPredicate::getMatching(TPred, FPred) || Swapped) {
if (Value *MatchOp =
getCommonOp(TI, FI, ICmpInst::isEquality(TPred), Swapped)) {
Value *NewSel = Builder.CreateSelect(Cond, OtherOpT, OtherOpF,
diff --git a/llvm/test/Transforms/InstCombine/select-cmp.ll b/llvm/test/Transforms/InstCombine/select-cmp.ll
index f7505bd85f89eb..7e5d5821d9f6a7 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 ult 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 ugt 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 ult 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:%.*]]
|
nikic
changed the title
|
Hi @artagnon, is this change expected to change floating point evaluation precision? We've found a case where this leads to different results. |
|
A bit of an incomplete hint; I think the most interesting diff in IR before and after this change is: and |
Seems a bit strange because the change did not touch FP compares. Could you kindly post an IR diff?
A strange diff, because the patch was only supposed to affect icmp samesign. |
|
This diff is a result of the whole -O3 optimization pipeline. It could be that the change here may affect further passes. We couldn't use |
|
A reduced test case, which, I believe, demonstrates a miscompile: If I read LLVM IR correctly, the branch condition in the "before" version of the diff corresponds to If my analysis is correct, please fix or revert soon. |
|
Created #124123 |
Unless there is a contraint on |
|
Single-function reproducer for define i1 @test(i32 %3) {
entry:
%4 = icmp slt i32 %3, 0
%.not.i.i = xor i1 true, %4
%5 = icmp samesign ult i32 %3, 131072
%spec.select = select i1 %.not.i.i, i1 %5, i1 %4
%6 = xor i1 %spec.select, true
%7 = icmp samesign ult i32 %3, -1
%or.cond = select i1 %4, i1 %7, i1 false
%or.cond2 = select i1 %6, i1 true, i1 %or.cond
ret i1 %or.cond2
} |
|
Reduced: define i1 @src(i1 %c, i32 %arg) {
%cmp1 = icmp samesign ult i32 %arg, 131072
%cmp2 = icmp slt i32 %arg, 0
%select = select i1 %c, i1 %cmp1, i1 %cmp2
ret i1 %select
}
define i1 @tgt(i1 %c, i32 %arg) {
%select.v = select i1 %c, i32 131072, i32 0
%select = icmp ult i32 %arg, %select.v
ret i1 %select
}The predicate in tgt should be slt, not ult. |
nikic
reviewed
Jan 23, 2025
| bool Swapped = ICmpInst::isRelational(FPred) && | ||
| CmpPredicate::getMatching( | ||
| TPred, ICmpInst::getSwappedCmpPredicate(FPred)); | ||
| if (CmpPredicate::getMatching(TPred, FPred) || Swapped) { |
There was a problem hiding this comment.
We use TPred below, instead of the matching predicate. Sorry for missing this obvious bug :(
There was a problem hiding this comment.
Thanks a lot for the investigation and test case: I was sick over the last few days, and couldn't do it myself :(
Will post a re-land shortly.
nikic
reviewed
Jan 23, 2025
| 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]] |
There was a problem hiding this comment.
The bug is also visible in this test case.
alexfh
added a commit
that referenced
this pull request
Jan 24, 2025
Reverts #122723 due to a miscompilation See #122723 (comment) for details and the test case.
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 24, 2025
… (#124123) Reverts llvm/llvm-project#122723 due to a miscompilation See llvm/llvm-project#122723 (comment) for details and the test case.
artagnon
added a commit
to artagnon/llvm-project
that referenced
this pull request
Jan 24, 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
added a commit
to artagnon/llvm-project
that referenced
this pull request
Jan 28, 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
added a commit
that referenced
this pull request
Jan 28, 2025
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]>
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.
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.