[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 9:48 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Eli Friedman wrote:
>>
>> 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.
>
> Nope, you're right. Turning this transform off.
>
> If you can find a way in which @preserve1 is wrong, I'll revert the whole
> patch. As it stands, it seems to me that we want something more generally
> applicable than MaintainNoSignedWrap.

I'm pretty sure @preserve1 is correct.  We discussed it pretty
extensively before the patch was committed.

-Eli