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

[Nick Lewycky]

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.

Nick