[llvm-commits] [llvm] r60275 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/apint-sub.ll test/Transforms/InstCombine/sdiv-1.ll test/Transforms/InstCombine/sub.ll

Bill Wendling isanbard at gmail.com
Sun Nov 30 23:59:28 PST 2008


On Nov 30, 2008, at 8:23 PM, Chris Lattner wrote:

> On Nov 29, 2008, at 7:42 PM, Bill Wendling wrote:
>
>> Author: void
>> Date: Sat Nov 29 21:42:12 2008
>> New Revision: 60275
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=60275&view=rev
>> Log:
>> Instcombine was illegally transforming -X/C into X/-C when either X
>> or C
>> overflowed on negation. This commit checks to make sure that neithe
>> C nor X
>> overflows. This requires that the RHS of X (a subtract instruction)
>> be a
>> constant integer.
>
> Hi Bill,
>
> I think that negate only overflows on minint, instead of:
>
>> +    ConstantInt *RHSNeg =
>> cast<ConstantInt>(ConstantExpr::getNeg(RHS));
>> +
>> +    // -X/C -> X/-C, if and only if negation doesn't overflow.
>> +    if ((RHS->getSExtValue() < 0 &&
>> +         RHS->getSExtValue() < RHSNeg->getSExtValue()) ||
>> +        (RHS->getSExtValue() > 0 &&
>> +         RHS->getSExtValue() > RHSNeg->getSExtValue())) {
>
> How about checking: "RHSNext != RHS" ?
>
I've since changed this. It's now:

         if ((RHS->getValue().isNegative() &&
              RHSNegAPI.slt(TwoToExp - 1)) ||
             (RHS->getValue().isNonNegative() &&
              RHSNegAPI.sgt(TwoToExp * NegOne))) {

This is what Hacker's Delight gave me (for -1*y). I can change it to  
RHSNeg != RHS if you think that this will always hold in this case  
(the non-negative part seems to be not necessary). What do you think?

> Also, please check the "if (Value *LHSNeg = dyn_castNegVal(Op0)) {"
> condition before computing the negation of the RHS.  This is a very
> uncommon transformation, so you shouldn't be fiddling with constants
> unless the pattern matches.
>
Done.

-bw




More information about the llvm-commits mailing list