[llvm] ae2322a - [InstSimplify] enhance simplifyWithOpReplaced() to allow more 'select' removal
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 21 14:03:45 PST 2023
Author: Sanjay Patel
Date: 2023-02-21T17:03:40-05:00
New Revision: ae2322a0dca9f8c39b1db51756c7155610fe668a
URL: https://github.com/llvm/llvm-project/commit/ae2322a0dca9f8c39b1db51756c7155610fe668a
DIFF: https://github.com/llvm/llvm-project/commit/ae2322a0dca9f8c39b1db51756c7155610fe668a.diff
LOG: [InstSimplify] enhance simplifyWithOpReplaced() to allow more 'select' removal
This is a generalization of a suggestion from issue #60799
that allows removing a redundant guard of an input value
via icmp+select. It should also solve issue #60801.
This only comes into play for a select with an equality
condition where we are trying to substitute a constant into
the false arm of a select. (A 'true' select arm substitution
allows "refinement", so it is not on this code path.)
The constant must be the same in the compare and the select,
and it must be a "binop absorber" (X op C = C). That query
currently includes 'or', 'and', and 'mul', so there are tests
for all of those opcodes.
We then use "impliesPoison" on the false arm binop and the
original "Op" to be replaced to ensure that the select is not
actually blocking poison from leaking. That could be
potentially expensive as we recursively test each operand, but
it is currently limited to a depth of 2. That's enough to catch
our motivating cases, but probably nothing more complicated
(although that seems unlikely).
I don't know how to generalize a proof for Alive2 for this, but
here's a positive and negative test example to help illustrate
the subtle logic differences of poison/undef propagation:
https://alive2.llvm.org/ce/z/Sz5K-c
Differential Revision: https://reviews.llvm.org/D144493
Added:
Modified:
llvm/lib/Analysis/InstructionSimplify.cpp
llvm/test/Transforms/InstSimplify/select.ll
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 0cd51fcac8ced..18b6aa442db66 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -4253,6 +4253,17 @@ static Value *simplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
if ((Opcode == Instruction::And || Opcode == Instruction::Or) &&
NewOps[0] == NewOps[1])
return NewOps[0];
+
+ // If we are substituting an absorber constant into a binop and extra
+ // poison can't leak if we remove the select -- because both operands of
+ // the binop are based on the same value -- then it may be safe to replace
+ // the value with the absorber constant. Examples:
+ // (Op == 0) ? 0 : (Op & -Op) --> Op & -Op
+ // (Op == 0) ? 0 : (Op * (binop Op, C)) --> Op * (binop Op, C)
+ // (Op == -1) ? -1 : (Op | (binop C, Op) --> Op | (binop C, Op)
+ if (RepOp == ConstantExpr::getBinOpAbsorber(Opcode, I->getType()) &&
+ impliesPoison(BO, Op))
+ return RepOp;
}
if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) {
diff --git a/llvm/test/Transforms/InstSimplify/select.ll b/llvm/test/Transforms/InstSimplify/select.ll
index 7ec6651ca77ba..79858ebdce862 100644
--- a/llvm/test/Transforms/InstSimplify/select.ll
+++ b/llvm/test/Transforms/InstSimplify/select.ll
@@ -1118,13 +1118,14 @@ define <2 x i32> @poison4(<2 x i1> %cond, <2 x i32> %x) {
ret <2 x i32> %v
}
+; 0 is the absorber constant for 'and'.
+; The 'select' can't block extra poison because both sides of 'and' have 'x' operand.
+
define i8 @replace_false_op_eq_neg_and(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq_neg_and(
-; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
-; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X]]
+; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X:%.*]]
; CHECK-NEXT: [[AND:%.*]] = and i8 [[NEG]], [[X]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[EQ0]], i8 0, i8 [[AND]]
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[AND]]
;
%eq0 = icmp eq i8 %x, 0
%neg = sub i8 0, %x
@@ -1133,13 +1134,13 @@ define i8 @replace_false_op_eq_neg_and(i8 %x) {
ret i8 %sel
}
+; same as above, but commute 'and'
+
define i8 @replace_false_op_eq_neg_and_commute(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq_neg_and_commute(
-; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
-; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X]]
+; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X:%.*]]
; CHECK-NEXT: [[AND:%.*]] = and i8 [[X]], [[NEG]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[EQ0]], i8 0, i8 [[AND]]
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[AND]]
;
%eq0 = icmp eq i8 %x, 0
%neg = sub i8 0, %x
@@ -1148,13 +1149,13 @@ define i8 @replace_false_op_eq_neg_and_commute(i8 %x) {
ret i8 %sel
}
+; same as above, but swap 'select'
+
define i8 @replace_false_op_ne_neg_and(i8 %x) {
; CHECK-LABEL: @replace_false_op_ne_neg_and(
-; CHECK-NEXT: [[NE0:%.*]] = icmp ne i8 [[X:%.*]], 0
-; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X]]
+; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X:%.*]]
; CHECK-NEXT: [[AND:%.*]] = and i8 [[NEG]], [[X]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[NE0]], i8 [[AND]], i8 0
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[AND]]
;
%ne0 = icmp ne i8 %x, 0
%neg = sub i8 0, %x
@@ -1163,13 +1164,13 @@ define i8 @replace_false_op_ne_neg_and(i8 %x) {
ret i8 %sel
}
+; same as above, but commute 'and' and swap 'select'
+
define i8 @replace_false_op_ne_neg_and_commute(i8 %x) {
; CHECK-LABEL: @replace_false_op_ne_neg_and_commute(
-; CHECK-NEXT: [[NE0:%.*]] = icmp ne i8 [[X:%.*]], 0
-; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X]]
+; CHECK-NEXT: [[NEG:%.*]] = sub i8 0, [[X:%.*]]
; CHECK-NEXT: [[AND:%.*]] = and i8 [[X]], [[NEG]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[NE0]], i8 [[AND]], i8 0
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[AND]]
;
%ne0 = icmp ne i8 %x, 0
%neg = sub i8 0, %x
@@ -1178,13 +1179,13 @@ define i8 @replace_false_op_ne_neg_and_commute(i8 %x) {
ret i8 %sel
}
+; the first binop can be anything as long as it has the common operand
+
define i8 @replace_false_op_eq_dec_and(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq_dec_and(
-; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
-; CHECK-NEXT: [[DEC:%.*]] = add i8 [[X]], -1
+; CHECK-NEXT: [[DEC:%.*]] = add i8 [[X:%.*]], -1
; CHECK-NEXT: [[AND:%.*]] = and i8 [[DEC]], [[X]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[EQ0]], i8 0, i8 [[AND]]
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[AND]]
;
%eq0 = icmp eq i8 %x, 0
%dec = add i8 %x, -1
@@ -1193,13 +1194,13 @@ define i8 @replace_false_op_eq_dec_and(i8 %x) {
ret i8 %sel
}
+; mul has the same absorber constant - "0"
+
define i8 @replace_false_op_eq_add_mul(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq_add_mul(
-; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
-; CHECK-NEXT: [[ADD:%.*]] = add i8 [[X]], 42
+; CHECK-NEXT: [[ADD:%.*]] = add i8 [[X:%.*]], 42
; CHECK-NEXT: [[MUL:%.*]] = mul i8 [[ADD]], [[X]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[EQ0]], i8 0, i8 [[MUL]]
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[MUL]]
;
%eq0 = icmp eq i8 %x, 0
%add = add i8 %x, 42
@@ -1208,13 +1209,13 @@ define i8 @replace_false_op_eq_add_mul(i8 %x) {
ret i8 %sel
}
+; or has a
diff erent absorber constant = "-1"
+
define i8 @replace_false_op_eq_shl_or(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq_shl_or(
-; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], -1
-; CHECK-NEXT: [[SHL:%.*]] = shl i8 [[X]], 3
+; CHECK-NEXT: [[SHL:%.*]] = shl i8 [[X:%.*]], 3
; CHECK-NEXT: [[OR:%.*]] = or i8 [[X]], [[SHL]]
-; CHECK-NEXT: [[SEL:%.*]] = select i1 [[EQ0]], i8 -1, i8 [[OR]]
-; CHECK-NEXT: ret i8 [[SEL]]
+; CHECK-NEXT: ret i8 [[OR]]
;
%eq0 = icmp eq i8 %x, -1
%shl = shl i8 %x, 3
@@ -1223,6 +1224,8 @@ define i8 @replace_false_op_eq_shl_or(i8 %x) {
ret i8 %sel
}
+; negative test - wrong cmp predicate
+
define i8 @replace_false_op_sgt_neg_and(i8 %x) {
; CHECK-LABEL: @replace_false_op_sgt_neg_and(
; CHECK-NEXT: [[EQ0:%.*]] = icmp sgt i8 [[X:%.*]], 0
@@ -1238,6 +1241,8 @@ define i8 @replace_false_op_sgt_neg_and(i8 %x) {
ret i8 %sel
}
+; negative test - the binop must use a compare operand
+
define i8 @replace_false_op_eq_shl_or_wrong_cmp_op(i8 %x, i8 %y) {
; CHECK-LABEL: @replace_false_op_eq_shl_or_wrong_cmp_op(
; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[Y:%.*]], -1
@@ -1253,6 +1258,8 @@ define i8 @replace_false_op_eq_shl_or_wrong_cmp_op(i8 %x, i8 %y) {
ret i8 %sel
}
+; negative test - can't have extra source of potential poison
+
define i8 @replace_false_op_eq_neg_and_leak1(i8 %x, i8 %y) {
; CHECK-LABEL: @replace_false_op_eq_neg_and_leak1(
; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
@@ -1268,6 +1275,8 @@ define i8 @replace_false_op_eq_neg_and_leak1(i8 %x, i8 %y) {
ret i8 %sel
}
+; negative test - can't have extra source of potential poison
+
define i8 @replace_false_op_eq_neg_and_leak2(i8 %x, i8 %y) {
; CHECK-LABEL: @replace_false_op_eq_neg_and_leak2(
; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
@@ -1283,6 +1292,8 @@ define i8 @replace_false_op_eq_neg_and_leak2(i8 %x, i8 %y) {
ret i8 %sel
}
+; negative test - can't have extra source of potential poison
+
define i8 @replace_false_op_eq_add_mul_leak3(i8 %x, i8 %y) {
; CHECK-LABEL: @replace_false_op_eq_add_mul_leak3(
; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], 0
@@ -1298,6 +1309,8 @@ define i8 @replace_false_op_eq_add_mul_leak3(i8 %x, i8 %y) {
ret i8 %sel
}
+; negative test - can't have extra source of potential poison
+
define i8 @replace_false_op_eq_shl_or_leak4(i8 %x, i8 %y) {
; CHECK-LABEL: @replace_false_op_eq_shl_or_leak4(
; CHECK-NEXT: [[EQ0:%.*]] = icmp eq i8 [[X:%.*]], -1
@@ -1313,6 +1326,8 @@ define i8 @replace_false_op_eq_shl_or_leak4(i8 %x, i8 %y) {
ret i8 %sel
}
+; negative test - wrong cmp constant
+
define i8 @replace_false_op_eq42_neg_and(i8 %x) {
; CHECK-LABEL: @replace_false_op_eq42_neg_and(
; CHECK-NEXT: [[EQ42:%.*]] = icmp eq i8 [[X:%.*]], 42
More information about the llvm-commits
mailing list