[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 =
----------------
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.


http://reviews.llvm.org/D12988