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

[Chen Li via llvm-commits]

chenli 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:
> 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.


http://reviews.llvm.org/D12988