[PATCH] InstCombine: simplify signed range checks
Philip Reames
listmail at philipreames.com
Tue Nov 25 17:54:52 PST 2014
On 11/25/2014 04:33 AM, Erik Eckstein wrote:
> Hi,
>
> this is a patch for InstCombine. It tries to simplify
> (icmp sgt x, -1) & (icmp sgt/sge n, x)
> to
> icmp ugt/uge n, x
>
> in case n is not negative.
I'm generally happy with this change, but have a few minor comments
inline. In the future, could you a) use phabricator and b) include more
context in the diffs?
>
>
> llvm-simplify-range-check.patch
>
>
> Index: lib/Transforms/InstCombine/InstCombine.h
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombine.h (revision 222749)
> +++ lib/Transforms/InstCombine/InstCombine.h (working copy)
> @@ -162,6 +162,7 @@
> Instruction *visitUDiv(BinaryOperator &I);
> Instruction *visitSDiv(BinaryOperator &I);
> Instruction *visitFDiv(BinaryOperator &I);
> + Value *simplifyRangeCheck(ICmpInst *Op0, ICmpInst *Op1);
> Value *FoldAndOfICmps(ICmpInst *LHS, ICmpInst *RHS);
> Value *FoldAndOfFCmps(FCmpInst *LHS, FCmpInst *RHS);
> Instruction *visitAnd(BinaryOperator &I);
> Index: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (revision 222749)
> +++ lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (working copy)
> @@ -785,6 +785,43 @@
> return nullptr;
> }
>
> +/// Try to fold (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x
Is there a reason you're only handling the upper bound phrased as a
sgt/sge? Do we canonicalize away the alternate slt/sle formation?
> +Value *InstCombiner::simplifyRangeCheck(ICmpInst *Cmp0, ICmpInst *Cmp1) {
This doesn't need to be a member function does it? A static function
would be preferred.
> + // Check the lower range comparison: x >= 0
> + ConstantInt *RangeStart = dyn_cast<ConstantInt>(Cmp0->getOperand(1));
> + if (!RangeStart)
> + return nullptr;
> +
> + if (!RangeStart->isMinusOne() || Cmp0->getPredicate() != ICmpInst::ICMP_SGT)
> + return nullptr;
It seems like you're relying on an existing canonicalization to signed x
> -1? If so, add that to your comment.
> +
> + // Only do the optimization if the compares are not used otherwise.
> + if (!Cmp0->hasOneUse() || !Cmp1->hasOneUse())
> + return nullptr;
You should remove this check and create a new instruction.
> +
> + // Check it both icmp compare against the same value.
> + if (Cmp1->getOperand(1) != Cmp0->getOperand(0))
> + return nullptr;
It might be clearer to use named temporises here:
Value* X1 = Cmp0->getOperand(0);
Value* X2 = Cmp1->getOperand(1)
Value* N = Cmp1->getOperand(0);
if (X1 != X2) return nullptr;
...etc..
> +
> + // Check the upper range comparison.
> + ICmpInst::Predicate NewPred;
> + switch (Cmp1->getPredicate()) {
> + case ICmpInst::ICMP_SGT: NewPred = ICmpInst::ICMP_UGT; break;
> + case ICmpInst::ICMP_SGE: NewPred = ICmpInst::ICMP_UGE; break;
> + default: return nullptr;
> + }
> +
> + // This simplification is only valid if the upper range is not negative.
> + bool IsNegative, IsNotNegative;
> + ComputeSignBit(Cmp1->getOperand(0), IsNotNegative, IsNegative, DL, 0, AT,
> + Cmp1, DT);
> + if (!IsNotNegative)
> + return nullptr;
> +
> + Cmp1->setPredicate(NewPred);
Please create a new icmp rather than modifying one in place.
> + return Cmp1;
> +}
> +
> /// FoldAndOfICmps - Fold (icmp)&(icmp) if possible.
> Value *InstCombiner::FoldAndOfICmps(ICmpInst *LHS, ICmpInst *RHS) {
> ICmpInst::Predicate LHSCC = LHS->getPredicate(), RHSCC = RHS->getPredicate();
> @@ -807,6 +844,14 @@
> if (Value *V = foldLogOpOfMaskedICmps(LHS, RHS, true, Builder))
> return V;
>
> + /// (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x
> + if (Value *V = simplifyRangeCheck(LHS, RHS))
> + return V;
> +
> + /// (icmp sgt/sge n, x) & (icmp sgt x, -1) --> icmp ugt/uge n, x
> + if (Value *V = simplifyRangeCheck(RHS, LHS))
> + return V;
> +
> // This only handles icmp of constants: (icmp1 A, C1) & (icmp2 B, C2).
> Value *Val = LHS->getOperand(0), *Val2 = RHS->getOperand(0);
> ConstantInt *LHSCst = dyn_cast<ConstantInt>(LHS->getOperand(1));
> Index: test/Transforms/InstCombine/range-check.ll
> ===================================================================
> --- test/Transforms/InstCombine/range-check.ll (revision 0)
> +++ test/Transforms/InstCombine/range-check.ll (working copy)
> @@ -0,0 +1,117 @@
> +; RUN: opt < %s -instcombine -S | FileCheck %s
> +
> +; Check simplification of
> +; (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x
> +
> +; CHECK-LABEL: define i1 @test1
> +; CHECK: %b = icmp ugt i32 %nn, %x
> +; CHECK: ret i1 %b
> +define i1 @test1(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sge i32 %x, 0
> + %b = icmp slt i32 %x, %nn
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @test2
> +; CHECK: %b = icmp uge i32 %nn, %x
> +; CHECK: ret i1 %b
> +define i1 @test2(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sgt i32 %x, -1
> + %b = icmp sle i32 %x, %nn
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @test3
> +; CHECK: %a = icmp ugt i32 %nn, %x
> +; CHECK: ret i1 %a
> +define i1 @test3(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sgt i32 %nn, %x
> + %b = icmp sge i32 %x, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @test4
> +; CHECK: %a = icmp uge i32 %nn, %x
> +; CHECK: ret i1 %a
> +define i1 @test4(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sge i32 %nn, %x
> + %b = icmp sge i32 %x, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; Negative tests
> +
> +; CHECK-LABEL: define i1 @negative1
> +; CHECK: %a = icmp
> +; CHECK: %b = icmp
> +; CHECK: %c = and i1 %a, %b
> +; CHECK: ret i1 %c
> +define i1 @negative1(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sgt i32 %nn, %x
> + %b = icmp sgt i32 %x, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @negative2
> +; CHECK: %a = icmp
> +; CHECK: %b = icmp
> +; CHECK: %c = and i1 %a, %b
> +; CHECK: ret i1 %c
> +define i1 @negative2(i32 %x, i32 %n) {
> + %a = icmp sgt i32 %n, %x
> + %b = icmp sge i32 %x, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @negative3
> +; CHECK: %a = icmp
> +; CHECK: %b = icmp
> +; CHECK: %c = and i1 %a, %b
> +; CHECK: ret i1 %c
> +define i1 @negative3(i32 %x, i32 %y, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sgt i32 %nn, %x
> + %b = icmp sge i32 %y, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @negative4
> +; CHECK: %a = icmp
> +; CHECK: %b = icmp
> +; CHECK: %c = and i1 %a, %b
> +; CHECK: ret i1 %c
> +define i1 @negative4(i32 %x, i32 %n) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp ne i32 %nn, %x
> + %b = icmp sge i32 %x, 0
> + %c = and i1 %a, %b
> + ret i1 %c
> +}
> +
> +; CHECK-LABEL: define i1 @negative5
> +; CHECK: %a = icmp
> +; CHECK: %b = icmp
> +; CHECK: %c = and i1 %a, %b
> +; CHECK: ret i1 %c
> +define i1 @negative5(i32 %x, i32 %n, i32 * %p) {
> + %nn = and i32 %n, 2147483647
> + %a = icmp sgt i32 %nn, %x
> + %b = icmp sge i32 %x, 0
> + %c = and i1 %a, %b
> + %ae = sext i1 %a to i32
> + store i32 %ae, i32 * %p
> + ret i1 %c
> +}
> +
>
>
> Erik
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141125/6e2caf03/attachment.html>
More information about the llvm-commits
mailing list