[PATCH] D122299: [InstCombine] Fix missing nsw flag when fold -(x-y) to y-x

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D122299#3402164 <https://reviews.llvm.org/D122299#3402164>, @bcl5980 wrote:

> In D122299#3402130 <https://reviews.llvm.org/D122299#3402130>, @lebedev.ri wrote:
>
>> In D122299#3402088 <https://reviews.llvm.org/D122299#3402088>, @bcl5980 wrote:
>>
>>> In D122299#3402030 <https://reviews.llvm.org/D122299#3402030>, @lebedev.ri wrote:
>>>
>>>> Must we do this? I was really hoping we wouldn't.
>>>> Does this actually unblock some measurable improvement?
>>>>
>>>> Consider: https://alive2.llvm.org/ce/z/XYfMSm
>>>> Both `sub`s have `NSW`, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6
>>>
>>> It comes from here https://reviews.llvm.org/D122013
>>
>>
>>
>>> Maybe we can enable it when IsTrulyNegation ?
>>
>> Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
>> But even then, the really big problem for `NSW` preservation is that every instruction inbetween that we manage to negate through affects the `NSW`: https://alive2.llvm.org/ce/z/4A--JC
>>
>>> Abandon is also OK for me.
>
>
>
> In D122299#3402130 <https://reviews.llvm.org/D122299#3402130>, @lebedev.ri wrote:
>
>> In D122299#3402088 <https://reviews.llvm.org/D122299#3402088>, @bcl5980 wrote:
>>
>>> In D122299#3402030 <https://reviews.llvm.org/D122299#3402030>, @lebedev.ri wrote:
>>>
>>>> Must we do this? I was really hoping we wouldn't.
>>>> Does this actually unblock some measurable improvement?
>>>>
>>>> Consider: https://alive2.llvm.org/ce/z/XYfMSm
>>>> Both `sub`s have `NSW`, so i believe the code will now produce: https://alive2.llvm.org/ce/z/vUseE6
>>>
>>> It comes from here https://reviews.llvm.org/D122013
>>
>>
>>
>>> Maybe we can enable it when IsTrulyNegation ?
>>
>> Oh, hm, that is also a required precondition apparently: https://alive2.llvm.org/ce/z/c9i8Go
>> But even then, the really big problem for `NSW` preservation is that every instruction inbetween that we manage to negate through affects the `NSW`: https://alive2.llvm.org/ce/z/4A--JC
>>
>>> Abandon is also OK for me.
>
> Thanks for the detail explaination. So if we really want this work, it should be
>
>   bool NSW = I->hasNoSignedWrap() && HasNSW && IsTrulyNegation && Depth == 0;
>   return Builder.CreateSub(I->getOperand(1), I->getOperand(0),
>                            I->getName() + ".neg", false, NSW);
>
> or we need to trace every recursive instruction is also NSW, am I right?

I believe that is the idea, yes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122299/new/

https://reviews.llvm.org/D122299