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

[Duncan Sands]

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.

> 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)?

>>> +    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).

Ciao,

Duncan.