[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

[Chris Lattner]

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" ?

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.

-Chris

>
> +      if (Value *LHSNeg = dyn_castNegVal(Op0)) {
> +        if (ConstantInt *CI = dyn_cast<ConstantInt>(LHSNeg)) {
> +          ConstantInt *CINeg =  
> cast<ConstantInt>(ConstantExpr::getNeg(CI));
> +
> +          if ((CI->getSExtValue() < 0 &&
> +               CI->getSExtValue() < CINeg->getSExtValue()) ||
> +              (CI->getSExtValue() > 0 &&
> +               CI->getSExtValue() > CINeg->getSExtValue()))
> +            return BinaryOperator::CreateSDiv(LHSNeg,
> +                                               
> ConstantExpr::getNeg(RHS));
> +        }
> +      }
> +    }
>   }
>
>   // If the sign bits of both operands are zero (i.e. we can prove  
> they are
>
> Modified: llvm/trunk/test/Transforms/InstCombine/apint-sub.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/apint-sub.ll?rev=60275&r1=60274&r2=60275&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/test/Transforms/InstCombine/apint-sub.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/apint-sub.ll Sat Nov 29  
> 21:42:12 2008
> @@ -3,7 +3,7 @@
> ;
>
> ; RUN: llvm-as < %s | opt -instcombine | llvm-dis | \
> -; RUN:   grep -v {sub i19 %Cok, %Bok} | not grep sub
> +; RUN:   grep -v {sub i19 %Cok, %Bok} | grep -v {sub i25 0, %Aok} |  
> not grep sub
> ; END.
>
> define i23 @test1(i23 %A) {
> @@ -107,8 +107,10 @@
> 	ret i51 %Y
> }
>
> -define i25 @test17(i25 %A) {
> -	%B = sub i25 0, %A		; <i25> [#uses=1]
> +; Can't fold subtract here because negation it might oveflow.
> +; PR3142
> +define i25 @test17(i25 %Aok) {
> +	%B = sub i25 0, %Aok		; <i25> [#uses=1]
> 	%C = sdiv i25 %B, 1234		; <i25> [#uses=1]
> 	ret i25 %C
> }
>
> Added: llvm/trunk/test/Transforms/InstCombine/sdiv-1.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/sdiv-1.ll?rev=60275&view=auto
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/test/Transforms/InstCombine/sdiv-1.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/sdiv-1.ll Sat Nov 29  
> 21:42:12 2008
> @@ -0,0 +1,22 @@
> +; RUN: llvm-as < %s | opt -instcombine -inline | llvm-dis | grep  
> {715827882} | count 2
> +; PR3142
> +
> +define i32 @a(i32 %X) nounwind readnone {
> +entry:
> +       %0 = sub i32 0, %X
> +       %1 = sdiv i32 %0, -3
> +       ret i32 %1
> +}
> +
> +define i32 @b(i32 %X) nounwind readnone {
> +entry:
> +       %0 = call i32 @a(i32 -2147483648)
> +       ret i32 %0
> +}
> +
> +define i32 @c(i32 %X) nounwind readnone {
> +entry:
> +       %0 = sub i32 0, -2147483648
> +       %1 = sdiv i32 %0, -3
> +       ret i32 %1
> +}
>
> Modified: llvm/trunk/test/Transforms/InstCombine/sub.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/sub.ll?rev=60275&r1=60274&r2=60275&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/test/Transforms/InstCombine/sub.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/sub.ll Sat Nov 29  
> 21:42:12 2008
> @@ -1,7 +1,7 @@
> ; This test makes sure that these instructions are properly  
> eliminated.
> ;
> ; RUN: llvm-as < %s | opt -instcombine | llvm-dis | \
> -; RUN:   grep -v {sub i32 %Cok, %Bok} | not grep sub
> +; RUN:   grep -v {sub i32 %Cok, %Bok} | grep -v {sub i32 0, %Aok} |  
> not grep sub
>
> define i32 @test1(i32 %A) {
> 	%B = sub i32 %A, %A		; <i32> [#uses=1]
> @@ -104,8 +104,10 @@
> 	ret i32 %Y
> }
>
> -define i32 @test17(i32 %A) {
> -	%B = sub i32 0, %A		; <i32> [#uses=1]
> +; Can't fold subtract here because negation it might oveflow.
> +; PR3142
> +define i32 @test17(i32 %Aok) {
> +	%B = sub i32 0, %Aok		; <i32> [#uses=1]
> 	%C = sdiv i32 %B, 1234		; <i32> [#uses=1]
> 	ret i32 %C
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits