-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[ValueTracking] Try to infer range of select from true and false values. #68256
Conversation
351aa87
to
6663d20
Compare
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms ChangesWhen computing range of Full diff: https://github.com/llvm/llvm-project/pull/68256.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 0736ef65b306519..4078376124b1750 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -8854,11 +8854,16 @@ ConstantRange llvm::computeConstantRange(const Value *V, bool ForSigned,
} else if (auto *II = dyn_cast<IntrinsicInst>(V))
CR = getRangeForIntrinsic(*II);
else if (auto *SI = dyn_cast<SelectInst>(V)) {
+ ConstantRange CRTrue = computeConstantRange(
+ SI->getTrueValue(), ForSigned, UseInstrInfo, AC, CtxI, DT, Depth + 1);
+ ConstantRange CRFalse = computeConstantRange(
+ SI->getFalseValue(), ForSigned, UseInstrInfo, AC, CtxI, DT, Depth + 1);
+ CR = CRTrue.unionWith(CRFalse);
+ // TODO: Return ConstantRange.
APInt Lower = APInt(BitWidth, 0);
APInt Upper = APInt(BitWidth, 0);
- // TODO: Return ConstantRange.
setLimitsForSelectPattern(*SI, Lower, Upper, IIQ);
- CR = ConstantRange::getNonEmpty(Lower, Upper);
+ CR = CR.intersectWith(ConstantRange::getNonEmpty(Lower, Upper));
} else if (isa<FPToUIInst>(V) || isa<FPToSIInst>(V)) {
APInt Lower = APInt(BitWidth, 0);
APInt Upper = APInt(BitWidth, 0);
diff --git a/llvm/test/Analysis/ValueTracking/constant-range-select.ll b/llvm/test/Analysis/ValueTracking/constant-range-select.ll
new file mode 100644
index 000000000000000..353fa7e1cf5dd33
--- /dev/null
+++ b/llvm/test/Analysis/ValueTracking/constant-range-select.ll
@@ -0,0 +1,15 @@
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=aa-eval -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
+
+@a = dso_local local_unnamed_addr global [10 x i32] zeroinitializer, align 4
+
+; CHECK-LABEL: Function: select_in_gep
+; CHECK: NoAlias: i32* %arrayidx, i32* getelementptr inbounds ([10 x i32], ptr @a, i64 0, i64 3)
+define i32 @select_in_gep(i1 %c) {
+entry:
+ %cond = select i1 %c, i64 2, i64 1
+ %0 = load i32, ptr getelementptr inbounds ([10 x i32], ptr @a, i64 0, i64 3), align 4
+ %arrayidx = getelementptr inbounds [10 x i32], ptr @a, i64 0, i64 %cond
+ store i32 %0, ptr %arrayidx, align 4
+ %1 = load i32, ptr getelementptr inbounds ([10 x i32], ptr @a, i64 0, i64 3), align 4
+ ret i32 %1
+}
diff --git a/llvm/test/Transforms/InstCombine/binop-select.ll b/llvm/test/Transforms/InstCombine/binop-select.ll
index a59e19897f061d1..6cd4132eadd77b9 100644
--- a/llvm/test/Transforms/InstCombine/binop-select.ll
+++ b/llvm/test/Transforms/InstCombine/binop-select.ll
@@ -324,12 +324,12 @@ define i32 @sub_sel_op1_use(i1 %b) {
; CHECK-LABEL: @sub_sel_op1_use(
; CHECK-NEXT: [[S:%.*]] = select i1 [[B:%.*]], i32 42, i32 41
; CHECK-NEXT: call void @use(i32 [[S]])
-; CHECK-NEXT: [[R:%.*]] = sub nsw i32 42, [[S]]
+; CHECK-NEXT: [[R:%.*]] = sub nuw nsw i32 42, [[S]]
; CHECK-NEXT: ret i32 [[R]]
;
%s = select i1 %b, i32 42, i32 41
call void @use(i32 %s)
- %r = sub nsw i32 42, %s
+ %r = sub nuw nsw i32 42, %s
ret i32 %r
}
|
SI->getTrueValue(), ForSigned, UseInstrInfo, AC, CtxI, DT, Depth + 1); | ||
ConstantRange CRFalse = computeConstantRange( | ||
SI->getFalseValue(), ForSigned, UseInstrInfo, AC, CtxI, DT, Depth + 1); | ||
CR = CRTrue.unionWith(CRFalse); |
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.
code wise looks fine.
Maybe add a todo for (or just implement) trying to use select condition. I.e we have something like:
select (icmp ule x, 10), x, 10
we can bound to [0, 10].
Something like:
const APInt * C;
if(match(SelectCond, m_ICmp(Pred, m_Specific(SelectTrue), m_APInt(C))) {
CRTrue = CRTrue.intersectWith(ConstantRange::makeExactICmpRegion(Pred), *C);
}
if(match(SelectCond, m_ICmp(Pred, m_Specific(SelectFalse), m_APInt(C))) {
CRFalse = CRFalse.intersectWith(ConstantRange::makeExactICmpRegion(getInverse(Pred), *C));
}
You could get a bit more sophisticated and do non-constants of just m_APInt(C)
and build a CR for the other operand.
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.
@goldsteinn I think setLimitsForSelectPattern
which is called below does what you describe.
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.
not quite, but the logic i'm proposing looks like it belongs better there. I'll make a patch for that.
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.
Still think the logic belongs here actually, but feel free to commit w.o adding, I'll likely add a follow up shortly after.
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.
@goldsteinn Maybe I didn't understand what you mean. But I tried the following test without my patch:
define i1 @foo(i32 %x) {
entry:
%cmp0_ = icmp ule i32 %x, 10
%select_ = select i1 %cmp0_, i32 %x, i32 10
%cmp1_ = icmp ule i32 %select_, 10
ret i1 %cmp1_
}
This simplifies to:
define i1 @foo(i32 %x) {
entry:
ret i1 true
}
I put a print statement right after a call to setLimitsForSelectPattern(*SI, Lower, Upper, IIQ)
, the return range is [0, 11)
.
Can you please demonstrate what's missing with another test case?
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.
@mgudim I believe the case @goldsteinn has in mind is where it's not a min/max pattern. So e.g. if you do icmp ule i32 %x, 20
then you know the result is <= 20, but it's not a min/max pattern.
LGTM. |
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.
I'm okay with this, but please keep this comment in mind:
llvm-project/llvm/include/llvm/Analysis/ValueTracking.h
Lines 903 to 904 in 5c4d35d
/// Determine the possible constant range of an integer or vector of integer | |
/// value. This is intended as a cheap, non-recursive check. |
If computeConstantRange() gets expanded substantially beyond this (e.g. if we did recursive binop calculations, which we currently intentionally don't do), then we'll have to re-evaluate how and where this function gets used. Being "essentially free" is part of its current feature set.
Right, maybe I can disable this by default and introduce a flag to allow expensive checks? Actually, BTW, right now there is no way to change I have a couple more patches where I make recursive calls in I was thinking that maybe in the long term we can move computation of Another idea: how about we cache queries in |
When computing range of `select` instruction, first compute the union of ranges of "True" and "False" operands of the `select` instruction.
6663d20
to
c499f0d
Compare
@nikic Is it OK to merge this patch now as it is? |
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. No concerns with this patch by itself.
When computing range of
select
instruction, first compute the union of ranges of "True" and "False" operands of theselect
instruction.