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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

At a high level, I find the notion of disabling folds to preserve nowrap flags somewhat concerning. Doing this means that an improvement in one place (which infers more nowrap flags) can disable a later optimization, because it now considers preserving those flags more important. We sometimes do this for correctness reasons (mainly in that an operation with nowrap flags cannot be guaranteed-not-poison), but do we have any precedent in InstCombine for doing this specifically to preserve the flags?

In this specific case, I think we should question the value of the entire transform, at least on the IR level. Is there some specific motivation for it (i.e. some chain of follow-on transforms it enables?) or are we just doing it because we can? Our KnownBits handling of sub is optimal, so for computeKnownBits() I believe it shouldn't matter whether the instruction is a sub or a xor (in cases where we convert one into the other based on known bits only). However, for ConstantRange for example we also have an optimal sub implementation, but xor is heavily approximated. Similarly SCEV does not understand xor, but understands sub. So I question the premise here that we want to prefer xor over sub as a canonical form. I believe on average, our support for sub is better (the only xor variant that has decently universal support is xor by minus one, aka not).


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

https://reviews.llvm.org/D129650