[ValueTracking] Try to infer range of select from true and false values.

[mgudim]

When computing range of select instruction, first compute the union of ranges of "True" and "False" operands of the select instruction.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Changes

When computing range of select instruction, first compute the union of ranges of "True" and "False" operands of the select instruction.

---

Full diff: https://github.com/llvm/llvm-project/pull/68256.diff

3 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+7-2)
• (added) llvm/test/Analysis/ValueTracking/constant-range-select.ll (+15)
• (modified) llvm/test/Transforms/InstCombine/binop-select.ll (+2-2)

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
 }
 

[goldsteinn]

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.

[mgudim]

@goldsteinn I think setLimitsForSelectPattern which is called below does what you describe.

[goldsteinn]

not quite, but the logic i'm proposing looks like it belongs better there. I'll make a patch for that.

[goldsteinn]

Still think the logic belongs here actually, but feel free to commit w.o adding, I'll likely add a follow up shortly after.

[mgudim]

@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?

[nikic]

@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.

[goldsteinn]

LGTM.

[nikic]

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.

[mgudim]

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, MaxAnalysisRecursionDepth is 6 - is it expensive?

BTW, right now there is no way to change MaxAnalysisRecursionDepth by cli option. Would we like to be able to?

I have a couple more patches where I make recursive calls in ValueTracking.cpp, so now is a good time to figure out what is the best approach to this.

I was thinking that maybe in the long term we can move computation of isKnownNonZero, computeConstantRange to Attributor framework?

Another idea: how about we cache queries in ValueTracking.cpp?

[mgudim]

@nikic Is it OK to merge this patch now as it is?

[nikic]

LGTM. No concerns with this patch by itself.