Add known and demanded bits support for zext nneg #70858
Conversation
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Philip Reames (preames) Changeszext nneg was recently added to the IR in #67982. This patch teaches Full diff: https://github.com/llvm/llvm-project/pull/70858.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index c303d261107eb19..8c6f43a8ea1438d 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1103,6 +1103,9 @@ static void computeKnownBitsFromOperator(const Operator *I,
assert(SrcBitWidth && "SrcBitWidth can't be zero");
Known = Known.anyextOrTrunc(SrcBitWidth);
computeKnownBits(I->getOperand(0), Known, Depth + 1, Q);
+ if (auto *Inst = dyn_cast<Instruction>(I);
+ Inst && Inst->getOpcode() == Instruction::ZExt && Inst->hasNonNeg())
+ Known.makeNonNegative();
Known = Known.zextOrTrunc(BitWidth);
break;
}
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
index cd6b017874e8d6c..1c85eb1b29adaa9 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -414,7 +414,10 @@ Value *InstCombinerImpl::SimplifyDemandedUseBits(Value *V, APInt DemandedMask,
if (SimplifyDemandedBits(I, 0, InputDemandedMask, InputKnown, Depth + 1))
return I;
assert(InputKnown.getBitWidth() == SrcBitWidth && "Src width changed?");
+ if (I->getOpcode() == Instruction::ZExt && I->hasNonNeg())
+ InputKnown.makeNonNegative();
Known = InputKnown.zextOrTrunc(BitWidth);
+
assert(!Known.hasConflict() && "Bits known to be one AND zero?");
break;
}
diff --git a/llvm/test/Transforms/InstCombine/zext.ll b/llvm/test/Transforms/InstCombine/zext.ll
index f595638ba9e3083..c60ff328277e66e 100644
--- a/llvm/test/Transforms/InstCombine/zext.ll
+++ b/llvm/test/Transforms/InstCombine/zext.ll
@@ -3,6 +3,7 @@
declare void @use1(i1)
declare void @use32(i32)
+declare void @use64(i32)
declare void @use_vec(<2 x i9>)
define i64 @test_sext_zext(i16 %A) {
@@ -762,3 +763,48 @@ define i32 @zext_icmp_eq0_no_shift(ptr %ptr ) {
%res = zext i1 %cmp to i32
ret i32 %res
}
+
+define i32 @zext_nneg_redundant_and(i8 %a) {
+; CHECK-LABEL: @zext_nneg_redundant_and(
+; CHECK-NEXT: [[A_I32:%.*]] = zext nneg i8 [[A:%.*]] to i32
+; CHECK-NEXT: ret i32 [[A_I32]]
+;
+ %a.i32 = zext nneg i8 %a to i32
+ %res = and i32 %a.i32, 127
+ ret i32 %res
+}
+
+; Negative test, the and can't be removed
+define i32 @zext_nneg_redundant_and_neg(i8 %a) {
+; CHECK-LABEL: @zext_nneg_redundant_and_neg(
+; CHECK-NEXT: [[B:%.*]] = and i8 [[A:%.*]], 127
+; CHECK-NEXT: [[B_I32:%.*]] = zext nneg i8 [[B]] to i32
+; CHECK-NEXT: ret i32 [[B_I32]]
+;
+ %b = and i8 %a, 127
+ %b.i32 = zext nneg i8 %b to i32
+ ret i32 %b.i32
+}
+
+define i64 @zext_nneg_signbit_extract(i32 %a) nounwind {
+; CHECK-LABEL: @zext_nneg_signbit_extract(
+; CHECK-NEXT: ret i64 0
+;
+ %b = zext nneg i32 %a to i64
+ %c = lshr i64 %b, 31
+ ret i64 %c
+}
+
+define i64 @zext_nneg_demanded_constant(i8 %a) nounwind {
+; CHECK-LABEL: @zext_nneg_demanded_constant(
+; CHECK-NEXT: [[B:%.*]] = zext nneg i8 [[A:%.*]] to i64
+; CHECK-NEXT: call void @use64(i64 [[B]]) #[[ATTR0:[0-9]+]]
+; CHECK-NEXT: [[C:%.*]] = and i64 [[B]], 126
+; CHECK-NEXT: ret i64 [[C]]
+;
+ %b = zext nneg i8 %a to i64
+ call void @use64(i64 %b)
+ %c = and i64 %b, 254
+ ret i64 %c
+}
+
diff --git a/llvm/test/Transforms/LoopVectorize/reduction.ll b/llvm/test/Transforms/LoopVectorize/reduction.ll
index 25352ee0991bade..f6c479ee92ce410 100644
--- a/llvm/test/Transforms/LoopVectorize/reduction.ll
+++ b/llvm/test/Transforms/LoopVectorize/reduction.ll
@@ -1204,7 +1204,7 @@ define i64 @reduction_with_phi_with_one_incoming_on_backedge(i16 %n, ptr %A) {
; CHECK-NEXT: [[MIN_ITERS_CHECK:%.*]] = icmp ult i16 [[SMAX]], 5
; CHECK-NEXT: br i1 [[MIN_ITERS_CHECK]], label [[SCALAR_PH:%.*]], label [[VECTOR_PH:%.*]]
; CHECK: vector.ph:
-; CHECK-NEXT: [[N_VEC:%.*]] = and i32 [[TMP1]], 65532
+; CHECK-NEXT: [[N_VEC:%.*]] = and i32 [[TMP1]], 32764
; CHECK-NEXT: [[DOTCAST:%.*]] = trunc i32 [[N_VEC]] to i16
; CHECK-NEXT: [[IND_END:%.*]] = or i16 [[DOTCAST]], 1
; CHECK-NEXT: br label [[VECTOR_BODY:%.*]]
@@ -1282,7 +1282,7 @@ define i64 @reduction_with_phi_with_two_incoming_on_backedge(i16 %n, ptr %A) {
; CHECK-NEXT: [[MIN_ITERS_CHECK:%.*]] = icmp ult i16 [[SMAX]], 5
; CHECK-NEXT: br i1 [[MIN_ITERS_CHECK]], label [[SCALAR_PH:%.*]], label [[VECTOR_PH:%.*]]
; CHECK: vector.ph:
-; CHECK-NEXT: [[N_VEC:%.*]] = and i32 [[TMP1]], 65532
+; CHECK-NEXT: [[N_VEC:%.*]] = and i32 [[TMP1]], 32764
; CHECK-NEXT: [[DOTCAST:%.*]] = trunc i32 [[N_VEC]] to i16
; CHECK-NEXT: [[IND_END:%.*]] = or i16 [[DOTCAST]], 1
; CHECK-NEXT: br label [[VECTOR_BODY:%.*]]
|
| @@ -1103,6 +1103,9 @@ static void computeKnownBitsFromOperator(const Operator *I, | |||
| assert(SrcBitWidth && "SrcBitWidth can't be zero"); | |||
| Known = Known.anyextOrTrunc(SrcBitWidth); | |||
| computeKnownBits(I->getOperand(0), Known, Depth + 1, Q); | |||
| if (auto *Inst = dyn_cast<Instruction>(I); | |||
| Inst && Inst->getOpcode() == Instruction::ZExt && Inst->hasNonNeg()) | |||
| Known.makeNonNegative(); | |||
There was a problem hiding this comment.
This is going to cause a conflict (and downstream assertion failures) if Known is known negative. You need to reset the known one bit -- or maybe adjust the makeNonNegative() API to do that itself, as this seems like an unnecessary footgun.
There was a problem hiding this comment.
It seems like doing it this way is missing an optimization. Be proving a conflict, haven't we proven the resulting value to be poison? If so, why are we not taking advantage of that to fold the value to poison?
Glancing through the code for KnownBits and SimplifyDemanded, I don't see any clear evidence of a consistent scheme being followed. Do you know the big picture here?
There was a problem hiding this comment.
It seems like doing it this way is missing an optimization. Be proving a conflict, haven't we proven the resulting value to be poison? If so, why are we not taking advantage of that to fold the value to poison?
Yes, a conflict implies the result is poison. I think the reason for the current approach is just "historical reasons" -- at some point somebody made the choice that we don't want to deal with conflicting KnownBits and we've stuck to that since. I'd be open to reevaluate that choice, but it's not something we can allow in just one place, it needs to be supported across all the KnownBits-based infrastructure-
Glancing through the code for KnownBits and SimplifyDemanded, I don't see any clear evidence of a consistent scheme being followed. Do you know the big picture here?
Usually one of 1. locally resolve the conflict (e.g. here unset the negative bit or don't set non-negative), 2. return unknown bits or 3. return zero.
See e.g.
for a very similar case to this one, which solves this by doing a!Known.isNegative() check first.
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
Outdated
Show resolved
Hide resolved
This fixes a miscompile introduced in the recent llvm#67982, and likely exposed in changes since to infer and leverage the same. No active bug reports as of yet. This was noticed in llvm#70858 (comment).
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This fixes a miscompile introduced in the recent #67982, and likely exposed in changes since to infer and leverage the same. No active bug reports as of yet. This was noticed in #70858 (comment).
zext nneg was recently added to the IR in llvm#67982. This patch teaches demanded bits and known bits about the semantics of the instruction, and adds a couple of test cases to illustrate basic functionality.
preames
force-pushed
the
pr-zext-nneg-known-bits
branch
from
5a53761
to
6e17593
Compare
| @@ -1103,6 +1103,9 @@ static void computeKnownBitsFromOperator(const Operator *I, | |||
| assert(SrcBitWidth && "SrcBitWidth can't be zero"); | |||
| Known = Known.anyextOrTrunc(SrcBitWidth); | |||
| computeKnownBits(I->getOperand(0), Known, Depth + 1, Q); | |||
| if (auto *Inst = dyn_cast<PossiblyNonNegInst>(I); | |||
There was a problem hiding this comment.
nit; imo should move this statement to before the if and use &&.
There was a problem hiding this comment.
Not sure what you're suggesting? Why would I want to expand the scope of the Inst variable?
There was a problem hiding this comment.
Guess the syntax if (A = expr; other_stuff...) is a pretty abnormal pattern and imo prone to be misread, but its a nit and nikics approved so no need to change if you feel otherwise.
| ; | ||
| %b = zext nneg i32 %a to i64 | ||
| %c = lshr i64 %b, 31 | ||
| ret i64 %c | ||
| } | ||
|
|
||
| define i64 @zext_nneg_demanded_constant(i8 %a) nounwind { | ||
| ; |
This fixes a miscompile introduced in the recent #67982, and likely exposed in changes since to infer and leverage the same. No active bug reports as of yet. This was noticed in llvm/llvm-project#70858 (comment).
zext nneg was recently added to the IR in #67982. This patch teaches
demanded bits and known bits about the semantics of the instruction, and
adds a couple of test cases to illustrate basic functionality.