[PATCH] InstCombine: simplify signed range checks
Erik Eckstein
eeckstein at apple.com
Wed Nov 26 07:36:48 PST 2014
Thanks for the detailed comments!
> In the future, could you a) use phabricator and b) include more context in the diffs?
Done -> http://reviews.llvm.org/D6422 <http://reviews.llvm.org/D6422>
I added my comments to this email there.
Thanks,
Erik
> On 26 Nov 2014, at 02:54, Philip Reames <listmail at philipreames.com> wrote:
>
>
> 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/20141126/e58de796/attachment.html>
More information about the llvm-commits
mailing list