[PATCH] D12988: [Bug 24848] Use range metadata to constant fold comparisons with constant values

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Analysis/InstructionSimplify.cpp:2392
@@ +2391,3 @@
+          ConstantInt *LowerBound =
+              mdconst::extract<ConstantInt>(Ranges->getOperand(2 * i + 0));
+          ConstantInt *UpperBound =
----------------
chenli wrote:
> chenli wrote:
> > sanjoy wrote:
> > > I don't think this is correct.  When an instruction has a `!range` like `[a,b) [c,d)`, the set of possible values is the union of all of these ranges.
> > > 
> > > For instance, I think you'll constant fold `x slt 10` to `true` here if the range of `x` is `[0,9) [100,200)`, even though `x` can be `120`.
> > > 
> > > I also don't think `FirstRangeContainsSecond` is a good abstraction, and its name is misleading.
> > > 
> > > I'd structure this bit of code as:
> > > 
> > > instead of
> > > 
> > > ```
> > >     if (Lower != Upper) {
> > >       ConstantRange LHS_CR = ConstantRange(Lower, Upper);
> > > 
> > >       if (Value *RetVal =
> > >           FirstRangeContainsSecond(RHS_CR, LHS_CR, RHS->getContext())) {
> > >         return RetVal;
> > >       }
> > >     }
> > > ```
> > > 
> > > ```
> > > ConstantRange LHS_CR = Lower == Upper ? ConstantRange::getFullSet() : ConstantRange(Lower, Upper);
> > > if (Instruction has !range MD) {
> > >   ConstantRange CR = NullSet;
> > >   for (auto Range : Inst->Ranges) {
> > >     CR = CR.union(RangeMD)
> > >   }
> > >   LHS_CR = LHS_CR.intersect(CR);
> > > }
> > > 
> > > if (!LHS_CR.isFullSet()) {
> > >   if (RHS_CR.contains(LHS_CR))
> > >   ... // the code as it is today
> > > }
> > > ```
> > Yes, you are correct. It should check the union of all the ranges here. I will fix it and update the patch.
> ```
> ConstantRange LHS_CR = Lower == Upper ? ConstantRange::getFullSet() : ConstantRange(Lower, Upper);
> if (Instruction has !range MD) {
>   ConstantRange CR = NullSet;
>   for (auto Range : Inst->Ranges) {
>     CR = CR.union(RangeMD)
>   }
>   LHS_CR = LHS_CR.intersect(CR);
> }
> ```
> The union part will actually give you false positive result. Consider you have a value of ranges [0, 3), [5, 7), and it is compared with == 4. Union the two ranges result a new range [0, 7), which can not be proven not equal to 4, while the original ranges can fold it. So instead of taking the union, it might be better to check each range separately, and return true if RHS_CR contains all ranges and false if RHS_CR.inverse() contains all.
Agreed.  If you can prove some fact about each of the individual ranges separately, then you can assume the fact to be true for the union of the ranges.

However, I suspect in most cases the `!range` metadata will contain exactly one range (in which case there is no loss of precision when taking the union of all the ranges) so the additional complexity may not be worth it.  I'll leave it to you to make the final decision on this.


http://reviews.llvm.org/D12988