[llvm-commits] [llvm] r120049 - in /llvm/trunk: lib/Target/README.txt lib/Transforms/InstCombine/InstCombineShifts.cpp test/Transforms/InstCombine/shift.ll

[Benjamin Kramer]

On 24.11.2010, at 01:37, Eli Friedman wrote:

> On Tue, Nov 23, 2010 at 12:33 PM, Benjamin Kramer
> <benny.kra at googlemail.com> wrote:
>> Author: d0k
>> Date: Tue Nov 23 14:33:57 2010
>> New Revision: 120049
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=120049&view=rev
>> Log:
>> 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.
>> 
>> Modified:
>>    llvm/trunk/lib/Target/README.txt
>>    llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp
>>    llvm/trunk/test/Transforms/InstCombine/shift.ll
>> 
>> Modified: llvm/trunk/lib/Target/README.txt
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/README.txt?rev=120049&r1=120048&r2=120049&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/README.txt (original)
>> +++ llvm/trunk/lib/Target/README.txt Tue Nov 23 14:33:57 2010
>> @@ -1704,8 +1704,8 @@
>>   %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.
>> 
>>  //===---------------------------------------------------------------------===//
>> 
>> 
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp?rev=120049&r1=120048&r2=120049&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp Tue Nov 23 14:33:57 2010
>> @@ -54,19 +54,17 @@
>>     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<BinaryOperator>(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<ConstantInt>(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;
>> +        }
> 
> Is this check correct for "X << (A % INT_MIN)"?

I don't see why INT_MIN should be handled differently here, can you elaborate?