Skip to content
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

[ValueTracking] Take into account whether zero is poison when computing CR for ct{t,l}z #122548

Merged
merged 2 commits into from
Jan 11, 2025
Merged

[ValueTracking] Take into account whether zero is poison when computing CR for ct{t,l}z #122548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35cc01e
[ValueTracking] Take into account wether zero is poison when computin…
goldsteinn Jan 10, 2025
8efb95f
Apply Suggestion
goldsteinn Jan 11, 2025
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
15 changes: 11 additions & 4 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9898,13 +9898,20 @@ static void setLimitsForBinOp(const BinaryOperator &BO, APInt &Lower,
}
}

static ConstantRange getRangeForIntrinsic(const IntrinsicInst &II) {
static ConstantRange getRangeForIntrinsic(const IntrinsicInst &II,
bool UseInstrInfo) {
unsigned Width = II.getType()->getScalarSizeInBits();
const APInt *C;
switch (II.getIntrinsicID()) {
case Intrinsic::ctpop:
case Intrinsic::ctlz:
case Intrinsic::cttz:
case Intrinsic::cttz: {
APInt Upper = APInt(Width, Width);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
APInt Upper = APInt(Width, Width);
APInt Upper(Width, Width);

if (!UseInstrInfo || !match(II.getArgOperand(1), m_One()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we normally consider the zero poison flag as part of UseInstrInfo.

Though this does make me wonder whether it may be time to replace this flag with an attribute (i.e. range not including zero on the argument). It would have a higher chance to be dropped, but require less special handling, and automatically enable things like CSE between ctz with different poison flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think attr would make more sense, its typically how we communicate poison generating constraints.

Upper += 1;
// Maximum of set/clear bits is the bit width.
return ConstantRange::getNonEmpty(APInt::getZero(Width), Upper);
}
case Intrinsic::ctpop:
// Maximum of set/clear bits is the bit width.
return ConstantRange::getNonEmpty(APInt::getZero(Width),
APInt(Width, Width) + 1);
Expand Down Expand Up @@ -10095,7 +10102,7 @@ ConstantRange llvm::computeConstantRange(const Value *V, bool ForSigned,
setLimitsForBinOp(*BO, Lower, Upper, IIQ, ForSigned);
CR = ConstantRange::getNonEmpty(Lower, Upper);
} else if (auto *II = dyn_cast<IntrinsicInst>(V))
CR = getRangeForIntrinsic(*II);
CR = getRangeForIntrinsic(*II, UseInstrInfo);
else if (auto *SI = dyn_cast<SelectInst>(V)) {
ConstantRange CRTrue = computeConstantRange(
SI->getTrueValue(), ForSigned, UseInstrInfo, AC, CtxI, DT, Depth + 1);
Expand Down
8 changes: 2 additions & 6 deletions llvm/test/Transforms/InstSimplify/call.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1582,9 +1582,7 @@ define i1 @ctlz_i1_non_poison_eq_false(i1 %x) {

define i1 @ctlz_i1_poison_eq_false(i1 %x) {
; CHECK-LABEL: @ctlz_i1_poison_eq_false(
; CHECK-NEXT: [[CT:%.*]] = call i1 @llvm.ctlz.i1(i1 [[X:%.*]], i1 true)
; CHECK-NEXT: [[CMP:%.*]] = icmp eq i1 [[CT]], false
; CHECK-NEXT: ret i1 [[CMP]]
; CHECK-NEXT: ret i1 true
;
%ct = call i1 @llvm.ctlz.i1(i1 %x, i1 true)
%cmp = icmp eq i1 %ct, false
Expand All @@ -1604,9 +1602,7 @@ define i1 @cttz_i1_non_poison_eq_false(i1 %x) {

define i1 @cttz_i1_poison_eq_false(i1 %x) {
; CHECK-LABEL: @cttz_i1_poison_eq_false(
; CHECK-NEXT: [[CT:%.*]] = call i1 @llvm.cttz.i1(i1 [[X:%.*]], i1 true)
; CHECK-NEXT: [[CMP:%.*]] = icmp eq i1 [[CT]], false
; CHECK-NEXT: ret i1 [[CMP]]
; CHECK-NEXT: ret i1 true
;
%ct = call i1 @llvm.cttz.i1(i1 %x, i1 true)
%cmp = icmp eq i1 %ct, false
Expand Down
Loading