[llvm-commits] [llvm] r75357 - in /llvm/trunk: include/llvm/Support/ConstantRange.h lib/Support/ConstantRange.cpp lib/Transforms/Scalar/PredicateSimplifier.cpp
Duncan Sands
baldrick at free.fr
Sat Jul 11 03:40:37 PDT 2009
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.
More information about the llvm-commits
mailing list