[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,

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

> +    case ICmpInst::ICMP_EQ:
> +      return ConstantRange(CR.getLower(), CR.getUpper());

can't this just return CR?

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

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

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.