[llvm-commits] [llvm] r75357 - in /llvm/trunk: include/llvm/Support/ConstantRange.h lib/Support/ConstantRange.cpp lib/Transforms/Scalar/PredicateSimplifier.cpp

[Nick Lewycky]

Duncan Sands wrote:
> Hi Nick,
> 
>> +  /// makeICmpRegion - Return the range of values that a value must be within
>> +  /// in order for the comparison specified by the predicate against range
>> +  /// Other to be true.
> 
> I find this unclear.  Suppose New is the returned range, then
> "New Pred Other" should be true.  I think it best to make it
> clear in the comment that you don't mean that "Other Pred New"
> should hold.

The property I'm looking for is that 'if your value is not within the 
range returned then it can't possibly satisfy the comparison'. I should 
write that then!

After some tweaking, the new comment reads:

/// makeICmpRegion - Produce the smallest range that contains all values 
that
/// might satisfy the comparison specified by Pred when compared to any 
value
/// contained within Other.

>> +    case ICmpInst::ICMP_EQ:
>> +      return ConstantRange(CR.getLower(), CR.getUpper());
> 
> can't this just return CR?

Obviously. Thank you.

>> +    case ICmpInst::ICMP_NE:
>> +      if (CR.isSingleElement())
>> +        return ConstantRange(CR.getUpper(), CR.getLower());
>> +      return ConstantRange(W);
> 
> Shouldn't this just return the complementary range?  I don't
> understand why you distinguish between the singleton case and
> the general case.

The rule is "if the value is not within the returned range, the 
comparison must be false". For the statement "x != 7" we can return [8, 
7) no problem.

Where CR is [7, 9) the condition to express is "x != 7 && x != 8". 
Returning [9, 7) would claim that the value 7 could never satisfy the 
condition, which isn't true (by choosing 8 out of CR).

>> +    case ICmpInst::ICMP_ULT:
>> +      return ConstantRange(APInt::getMinValue(W), CR.getUnsignedMax());
> 
> I think this is wrong if the range wraps around.  For example, suppose
> (8 bit case) CR = [100, 10).  Then you return [0, 10), but in fact you
> need to return the empty set since the original range contains 0.

That's not what getUnsignedMax does. Your example would be what happens 
if we used CR.getUpper.

CR.getUnsignedMax returns the unsigned-largest value within the range. 
For [100, 10) that equals INT_MAX. Thus it returns [0, INT_MAX) which 
correctly excludes the only value that couldn't possibly satisfy such a 
comparison.

Nick

> Lots of similar comments for the rest.  Presumably I misunderstood
> something about constant ranges, because the logic doesn't make much
> sense to me.
> 
> The ConstantRange::contains method looks fine.
> 
> Ciao,
> 
> Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>