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

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added a comment.

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?


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

https://reviews.llvm.org/D122299