-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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.
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
[CVP] Flip signedness icmp predicate in use level #69948
Conversation
@llvm/pr-subscribers-llvm-transforms Author: XChy (XChy) ChangesResolve #69928. Full diff: https://github.com/llvm/llvm-project/pull/69948.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
index 523196e5e6eab71..9043c434313fedc 100644
--- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
+++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
@@ -294,8 +294,9 @@ static bool processICmp(ICmpInst *Cmp, LazyValueInfo *LVI) {
ICmpInst::Predicate UnsignedPred =
ConstantRange::getEquivalentPredWithFlippedSignedness(
- Cmp->getPredicate(), LVI->getConstantRange(Cmp->getOperand(0), Cmp),
- LVI->getConstantRange(Cmp->getOperand(1), Cmp));
+ Cmp->getPredicate(),
+ LVI->getConstantRangeAtUse(Cmp->getOperandUse(0)),
+ LVI->getConstantRangeAtUse(Cmp->getOperandUse(1)));
if (UnsignedPred == ICmpInst::Predicate::BAD_ICMP_PREDICATE)
return false;
diff --git a/llvm/test/Transforms/CorrelatedValuePropagation/icmp.ll b/llvm/test/Transforms/CorrelatedValuePropagation/icmp.ll
index c4f0ade39942a76..24acd528e6dcde5 100644
--- a/llvm/test/Transforms/CorrelatedValuePropagation/icmp.ll
+++ b/llvm/test/Transforms/CorrelatedValuePropagation/icmp.ll
@@ -1246,3 +1246,78 @@ declare <2 x i8> @llvm.umin.v2i8(<2 x i8>, <2 x i8>)
declare <2 x i8> @llvm.umax.v2i8(<2 x i8>, <2 x i8>)
attributes #4 = { noreturn }
+
+define i64 @test_select_flip(i64 noundef %arg) {
+; CHECK-LABEL: @test_select_flip(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[CMP1:%.*]] = icmp ult i64 [[ARG:%.*]], 1000
+; CHECK-NEXT: [[CMP2:%.*]] = icmp ult i64 [[ARG]], 100
+; CHECK-NEXT: [[SELECT:%.*]] = select i1 [[CMP1]], i1 [[CMP2]], i1 false
+; CHECK-NEXT: br i1 [[SELECT]], label [[GOOD:%.*]], label [[BAD:%.*]]
+; CHECK: bad:
+; CHECK-NEXT: ret i64 1
+; CHECK: good:
+; CHECK-NEXT: ret i64 0
+;
+entry:
+ %cmp1 = icmp ult i64 %arg, 1000
+ %cmp2 = icmp slt i64 %arg, 100
+ %select = select i1 %cmp1, i1 %cmp2, i1 false
+ br i1 %select, label %good, label %bad
+
+bad:
+ ret i64 1
+
+good:
+ ret i64 0
+}
+
+define i64 @test_select_flip_fail1(i64 noundef %arg) {
+; CHECK-LABEL: @test_select_flip_fail1(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[CMP1:%.*]] = icmp slt i64 [[ARG:%.*]], 1000
+; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i64 [[ARG]], 100
+; CHECK-NEXT: [[SELECT:%.*]] = select i1 [[CMP1]], i1 [[CMP2]], i1 false
+; CHECK-NEXT: br i1 [[SELECT]], label [[GOOD:%.*]], label [[BAD:%.*]]
+; CHECK: bad:
+; CHECK-NEXT: ret i64 1
+; CHECK: good:
+; CHECK-NEXT: ret i64 0
+;
+entry:
+ %cmp1 = icmp slt i64 %arg, 1000
+ %cmp2 = icmp slt i64 %arg, 100
+ %select = select i1 %cmp1, i1 %cmp2, i1 false
+ br i1 %select, label %good, label %bad
+
+bad:
+ ret i64 1
+
+good:
+ ret i64 0
+}
+
+define i64 @test_select_flip_fail2(i64 noundef %arg) {
+; CHECK-LABEL: @test_select_flip_fail2(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[CMP1:%.*]] = icmp ult i64 [[ARG:%.*]], 1000
+; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i64 [[ARG]], 100
+; CHECK-NEXT: [[SELECT:%.*]] = select i1 [[CMP1]], i1 false, i1 [[CMP2]]
+; CHECK-NEXT: br i1 [[SELECT]], label [[GOOD:%.*]], label [[BAD:%.*]]
+; CHECK: bad:
+; CHECK-NEXT: ret i64 1
+; CHECK: good:
+; CHECK-NEXT: ret i64 0
+;
+entry:
+ %cmp1 = icmp ult i64 %arg, 1000
+ %cmp2 = icmp slt i64 %arg, 100
+ %select = select i1 %cmp1, i1 false, i1 %cmp2
+ br i1 %select, label %good, label %bad
+
+bad:
+ ret i64 1
+
+good:
+ ret i64 0
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me, but can you please adjust the test coverage so the second condition is not implied (i.e. folds to true) by the first? In that case the transform isn't really meaningful.
As you mark #69928 as fixed, you should also add the test case from there.
%cmp1 = icmp ult i64 %arg, 1000 | ||
%cmp2 = icmp slt i64 %arg, 100 | ||
%select = select i1 %cmp1, i1 %cmp2, i1 false | ||
br i1 %select, label %good, label %bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The br is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've removed it everywhere but here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a copy from the original issue, so it contains br.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a literal copy, just the relevant part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from test nits.
Resolve #69928.
Alive2 Proof.