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

Fri Jul 22 12:43:17 PDT 2022

nikic added a comment.

I think the right starting point here is to revert rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd <https://reviews.llvm.org/rG79bb915fb60b2cd220d89e3bb54f67abb8cdb7bd>.

> This reminds me of turning 'add' into 'or' with no common bits set (no carries) - that led to a long series of work-arounds to match new patterns with 'or'.
> I'm not sure what (if any) SCEV changes were made to accommodate that transform, but it makes sense to me if we want to avoid that with sub->xor.

We have special SCEV handling for this here: https://github.com/llvm/llvm-project/blob/7068aa98412ade19a34b7ed126f4669f581b2311/llvm/lib/Analysis/ScalarEvolution.cpp#L7599 And I believe we have a bunch of haveNoCommonBitsSet() calls in various places to undo the add->or fold. I have some doubts about the value of the add->or canonicalization as well, but the justification is stronger there, and we already have necessary mitigations in place.

> That raises the question: do we want to canonicalize xor to sub?

That would be the principled thing to do, but I'm not sure it would matter much in practice.

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

https://reviews.llvm.org/D129650