From c21a821e9f044004bc504dd7bfdfaecb9d4af16b Mon Sep 17 00:00:00 2001 From: Benjamin Kramer Date: Tue, 23 Nov 2010 20:33:57 +0000 Subject: [PATCH] The srem -> urem transform is not safe for any divisor that's not a power of two. E.g. -5 % 5 is 0 with srem and 1 with urem. Also addresses Frits van Bommel's comments. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@120049 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/README.txt | 4 ++-- .../InstCombine/InstCombineShifts.cpp | 20 +++++++++---------- test/Transforms/InstCombine/shift.ll | 6 +++--- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/lib/Target/README.txt b/lib/Target/README.txt index b98531ca859..38f5af642d7 100644 --- a/lib/Target/README.txt +++ b/lib/Target/README.txt @@ -1704,8 +1704,8 @@ Missed instcombine transformation: %384 = shl i64 %381, %383 ; [#uses=1] %385 = icmp slt i32 %tmp14.i, 64 ; [#uses=1] -The srem can be transformed to an and because if x is negative, the shift is -undefined. Testcase derived from 403.gcc. +The srem can be transformed to an and because if %tmp14.i is negative, the +shift is undefined. Testcase derived from 403.gcc. //===---------------------------------------------------------------------===// diff --git a/lib/Transforms/InstCombine/InstCombineShifts.cpp b/lib/Transforms/InstCombine/InstCombineShifts.cpp index 7969dbfec04..e52f2013fe5 100644 --- a/lib/Transforms/InstCombine/InstCombineShifts.cpp +++ b/lib/Transforms/InstCombine/InstCombineShifts.cpp @@ -54,19 +54,17 @@ Instruction *InstCombiner::commonShiftTransforms(BinaryOperator &I) { if (Instruction *Res = FoldShiftByConstant(Op0, CUI, I)) return Res; - // X shift (A srem B) -> X shift (A urem B) iff B is positive. + // X shift (A srem B) -> X shift (A and B-1) iff B is a power of 2. // Because shifts by negative values are undefined. if (BinaryOperator *BO = dyn_cast(Op1)) - if (BO->getOpcode() == Instruction::SRem && BO->getType()->isIntegerTy()) { - // Make sure the divisor's sign bit is zero. - APInt Mask = APInt::getSignBit(BO->getType()->getPrimitiveSizeInBits()); - if (MaskedValueIsZero(BO->getOperand(1), Mask)) { - Value *URem = Builder->CreateURem(BO->getOperand(0), BO->getOperand(1), - BO->getName()); - I.setOperand(1, URem); - return &I; - } - } + if (BO->hasOneUse() && BO->getOpcode() == Instruction::SRem) + if (ConstantInt *CI = dyn_cast(BO->getOperand(1))) + if (CI->getValue().isPowerOf2()) { + Constant *C = ConstantInt::get(BO->getType(), CI->getValue()-1); + Value *Rem = Builder->CreateAnd(BO->getOperand(0), C, BO->getName()); + I.setOperand(1, Rem); + return &I; + } return 0; } diff --git a/test/Transforms/InstCombine/shift.ll b/test/Transforms/InstCombine/shift.ll index 6bebca9cc85..8d1c82991fd 100644 --- a/test/Transforms/InstCombine/shift.ll +++ b/test/Transforms/InstCombine/shift.ll @@ -443,12 +443,12 @@ entry: } define i32 @test38(i32 %x) nounwind readnone { -entry: %rem = srem i32 %x, 32 %shl = shl i32 1, %rem ret i32 %shl ; CHECK: @test38 -; CHECK-NOT: srem -; CHECK: ret i32 +; CHECK-NEXT: and i32 %x, 31 +; CHECK-NEXT: shl i32 1 +; CHECK-NEXT: ret i32 } -- 2.34.1