[PATCH] D129650: [InstCombine] change conditions for transform of sub to xor

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a subscriber: uabelho.
bjope added a comment.

In D129650#3666246 <https://reviews.llvm.org/D129650#3666246>, @spatel wrote:

> In D129650#3666235 <https://reviews.llvm.org/D129650#3666235>, @syzaara wrote:
>
>> In D129650#3666095 <https://reviews.llvm.org/D129650#3666095>, @spatel wrote:
>>
>>> In D129650#3666042 <https://reviews.llvm.org/D129650#3666042>, @syzaara wrote:
>>>
>>>> Note that we do not recognize the xor as a subtract of 8 SCEV expression. This prevents further simplifications on expressions using _sub_tmp4, because it is now represented as a symbol rather than a recognized SCEV operation.
Would it be possible to revert this until we teach SCEV how to handle the XOR?
>>>
>>> Please correct me if I've misunderstood: would **this** patch solve the problem that you are seeing? If we want to revert, that would be 79bb915fb60b <https://reviews.llvm.org/rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd> ?
>>
>> Oh sorry! Yes, I meant to post this comment for 79bb915fb60b <https://reviews.llvm.org/rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd>. And I have double checked that this patch solves the issue. Looking forward to this being committed, if we are not reverting 79bb915fb60b <https://reviews.llvm.org/rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd>. Thanks.
>
> Thanks for confirming.
>
> Ping @nikic - are you ok with this patch? (and follow-up to remove the fold entirely?)

This patch is better than nothing IMHO (the regressions we currently see without either applying this patch or reverting the recent patches involving sub->xor transforms are a bit annoying).
I also think that it is a bit weird (or rather hard to follow) if we only canonicalize based on if those NSW/NUW attributes are present or not (at least if we also want to canonicalize xor->sub for the opposite condition). So some kind of follow up to either just keep (or canonicalize) to sub up until instruction selection could be nice. If there are motivating examples for using xor instead of sub in the middle-end, then we might go in the other direction if we first make sure SCEV handles the xor in the same way as if it had been a sub.

Reverting rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd <https://reviews.llvm.org/rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd> is also an acceptable solution. Afaict it has only caused regressions, but I haven't seen (or heard about) any benchmarks being improved by that patch.


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

https://reviews.llvm.org/D129650