[llvm-commits] [llvm] r137572 - in /llvm/trunk: lib/Transforms/InstCombine/InstructionCombining.cpp test/Transforms/InstCombine/nsw.ll

[Eli Friedman]

On Sat, Aug 13, 2011 at 8:41 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Author: nicholas
> Date: Sat Aug 13 22:41:33 2011
> New Revision: 137572
>
> URL: http://llvm.org/viewvc/llvm-project?rev=137572&view=rev
> Log:
> Don't attempt to add 'nsw' when intermediate instructions had no such guarantee.
>
> Modified:
>    llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
>    llvm/trunk/test/Transforms/InstCombine/nsw.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=137572&r1=137571&r2=137572&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp Sat Aug 13 22:41:33 2011
> @@ -146,7 +146,6 @@
>   return !Overflow;
>  }
>
> -
>  /// SimplifyAssociativeOrCommutative - This performs a few simplifications for
>  /// operators which are associative or commutative:
>  //
> @@ -197,7 +196,10 @@
>           I.setOperand(1, V);
>           // Conservatively clear the optional flags, since they may not be
>           // preserved by the reassociation.
> -          if (MaintainNoSignedWrap(I, B, C)) {
> +          if (MaintainNoSignedWrap(I, B, C) &&
> +             (!Op0 || (isa<BinaryOperator>(Op0) && Op0->hasNoSignedWrap()))) {
> +            // Note: this is only valid because SimplifyBinOp doesn't look at
> +            // the operands to Op0.
>             I.clearSubclassOptionalData();
>             I.setHasNoSignedWrap(true);
>           } else {
> @@ -292,10 +294,11 @@
>         I.setOperand(1, Folded);
>         // Conservatively clear the optional flags, since they may not be
>         // preserved by the reassociation.
> -        if (MaintainNoSignedWrap(I, C1, C2)) {
> +        if (MaintainNoSignedWrap(I, C1, C2) && Op0->hasNoSignedWrap() &&
> +           Op1->hasNoSignedWrap()) {
> +          New->setHasNoSignedWrap(true);
>           I.clearSubclassOptionalData();
>           I.setHasNoSignedWrap(true);
> -          New->setHasNoSignedWrap(true);
>         } else {
>           I.clearSubclassOptionalData();
>         }
>
> Modified: llvm/trunk/test/Transforms/InstCombine/nsw.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/nsw.ll?rev=137572&r1=137571&r2=137572&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/nsw.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/nsw.ll Sat Aug 13 22:41:33 2011
> @@ -46,10 +46,38 @@
>   ret i32 %add3
>  }
>
> +; CHECK: @preserve2
> +; CHECK: add nsw i8 %A, %B
> +; CHECK: add nsw i8
> +define i8 @preserve2(i8 %A, i8 %B) nounwind {
> +  %x = add nsw i8 %A, 10
> +  %y = add nsw i8 %B, 10
> +  %add = add nsw i8 %x, %y
> +  ret i8 %add
> +}

Is this transform actually safe?  Take %A=-70, %B = -70.  Unless I'm
mistaken, the original operations (-70 + 10 and -60+-60) don't
overflow, but the new operations (-70 + -70 and 116 + 20) both
overflow.

-Eli