Skip to content

Add known and demanded bits support for zext nneg #70858

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.

Sign up for GitHub

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

Merged
preames merged 3 commits into from
Nov 7, 2023
Merged

Add known and demanded bits support for zext nneg #70858

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e17593
Add known and demanded bits support for zext nneg
preames Oct 30, 2023
138217f
Avoid conflicts in known bits
preames Nov 6, 2023
d1729b4
Update zext.ll
preames Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@goldsteinn goldsteinn Nov 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; imo should move this statement to before the if and use &&.

Copy link
Collaborator Author

@preames preames Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you're suggesting? Why would I want to expand the scope of the Inst variable?

Copy link
Contributor

@goldsteinn goldsteinn Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Inst && Inst->hasNonNeg() && !Known.isNegative())
Known.makeNonNegative();
Copy link
Contributor

@nikic nikic Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@preames preames Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@nikic nikic Nov 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 426 in 6e17593

if (isKnownNonNegative && !Known.isNegative())
for a very similar case to this one, which solves this by doing a !Known.isNegative() check first.

Known = Known.zextOrTrunc(BitWidth);
break;
}
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,11 @@ Value *InstCombinerImpl::SimplifyDemandedUseBits(Value *V, APInt DemandedMask,
return I;
}
assert(InputKnown.getBitWidth() == SrcBitWidth && "Src width changed?");
if (I->getOpcode() == Instruction::ZExt && I->hasNonNeg() &&
!InputKnown.isNegative())
InputKnown.makeNonNegative();
Known = InputKnown.zextOrTrunc(BitWidth);

assert(!Known.hasConflict() && "Bits known to be one AND zero?");
break;
}
Expand Down
12 changes: 5 additions & 7 deletions llvm/test/Transforms/InstCombine/zext.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,9 +795,8 @@ define i16 @zext_nneg_flag_drop(i8 %x, i16 %y) {

define i32 @zext_nneg_redundant_and(i8 %a) {
; CHECK-LABEL: @zext_nneg_redundant_and(
; CHECK-NEXT: [[TMP1:%.*]] = and i8 [[A:%.*]], 127
; CHECK-NEXT: [[RES:%.*]] = zext i8 [[TMP1]] to i32
; CHECK-NEXT: ret i32 [[RES]]
; 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
Expand All @@ -818,20 +817,19 @@ define i32 @zext_nneg_redundant_and_neg(i8 %a) {

define i64 @zext_nneg_signbit_extract(i32 %a) nounwind {
; CHECK-LABEL: @zext_nneg_signbit_extract(
; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[A:%.*]], 31
; CHECK-NEXT: [[C:%.*]] = zext i32 [[TMP1]] to i64
; CHECK-NEXT: ret i64 [[C]]
; 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 {
;
Copy link
Contributor

@nikic nikic Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray semicolon?

; 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]], 254
; CHECK-NEXT: [[C:%.*]] = and i64 [[B]], 126
; CHECK-NEXT: ret i64 [[C]]
;
%b = zext nneg i8 %a to i64
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/Transforms/LoopVectorize/reduction.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:%.*]]
Expand Down Expand Up @@ -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:%.*]]
Expand Down