[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,
> 
>> 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!
> 
> so if This = makeICmpRegion(Other) then for every T not in the range
> This and for every O in the range Other, T Pred O is not true ?
> 
> I find this confusing - two negations in one definition is too much
> for me!  Suppose you defined makeICmpRegion2 by:
>    if This = makeICmpRegion2(Other) then for every T in This and for
>    every O in Other, T pred O is true.  "This" is chosen maximal.
> That seems simpler to me (but maybe not to you :) )  Then these methods
> satisfy:
>    makeICmpRegion(Other, Pred) =
>      complement makeICmpRegion2(Other, opposite Pred),
> so there would be no problem doing everything in terms of
> makeICmpRegion2 instead.  I think the routine defined for the world in
> ConstantRange.h should be "makeICmpRegion2" (probably renamed to
> makeICmpRegion) and not the current version, because it is conceptually
> simpler.

Your proposed function may also be useful (and certainly has more 
obvious behaviour to explain!) but the two can't be implemented in terms 
of each other.

The existing one solves for range X in 'for all x in X, there exists a y 
in Y such that icmp op x, y is true'. As a worked example:
makeICmpRegion([5, 10), ICMP_ULT) = [0, 9). I can choose '9' out of Y 
and that means the value '8' belongs in X because 8 ult 9 is true. Every 
value that might make the comparison true is included.

The function you're suggesting one that solves for range X in 'for all x 
in X and for all y in Y such that icmp op x, y is true'. 
alternateMakeICmpRegion([5, 10), ICMP_ULT) = [0, 4). I can choose '5' 
out of Y and that means that '5' does not belong in X because 5 ult 5 is 
false. Only values that guarantee the comparison is true are included.

Neither one is the complement of the other. I hope that's finally clear. 
I think we need a new function name very badly, but if you reread the 
comment below, you'll probably find it more clear now that you aren't 
still thinking of the alternate behaviour. :)

>> 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.
> 
> This is indeed an improvement, but since the exact semantics are
> important then I think you should be even more explicit, using
> quantifiers as above, perhaps at the end of the comment.  By the way,
> does a unique smallest range always exist (the comment implicitly
> suggests that one does)?

Yes it's unique.

>>>> +    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.
> 
> I had misunderstood how this method is defined.  This also explains
> my other comments :)  By the way, if CR is the empty range then you
> should return the empty range here (since you return the smallest
> possible range).

Good point. I should audit this stuff for EmptySet; both LoopVR and 
PredicateSimplifier assumed they'd never see empty sets.

Nick