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

Chen Li via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 18 23:57:37 PDT 2015


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:
> 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.
I think that makes sense. I will take the general approach (union) first, and improve it if it shows profitable in practice in the future.


http://reviews.llvm.org/D12988





More information about the llvm-commits mailing list