[PATCH] D109151: [InstCombine] Convert xor (ashr X, BW-1), C -> select(X >=s 0, C, ~C)

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3436
+        *CA == X->getType()->getScalarSizeInBits() - 1 &&
+        !C1->isAllOnesValue() && !C1->isZeroValue()) {
+      Value *ICmp = Builder.CreateICmpSGE(X, ConstantInt::get(X->getType(), 0));
----------------
spatel wrote:
> "xor X, 0"  - seems like that should be an assert because it means this instruction escaped from instsimplify.
> 
> "xor X, -1" - I think we should handle this too for consistency. The transform holds for any value unless I'm missing something:
> https://alive2.llvm.org/ce/z/R7qEeT
> (If that's right, please add a test so we know we are not or will not conflict with some other transform.)
Yeah that apparently comes up a few times in the existing tests.

>From what I can tell, we would convert a `select c, -1, 0` to a `sext c`:
https://godbolt.org/z/zKePx154f
And a `(x >s -1) ? -1 : 0 -> not (ashr x, 31)`:
https://github.com/llvm/llvm-project/blob/c01b76e733d6e5e2d21e4277dceaa1f319794c6a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L1408

It makes sense to treat "not" as special to me, more canonical than the other folds here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3437
+        !C1->isAllOnesValue() && !C1->isZeroValue()) {
+      Value *ICmp = Builder.CreateICmpSGE(X, ConstantInt::get(X->getType(), 0));
+      return SelectInst::Create(ICmp, Op1, Builder.CreateNot(Op1));
----------------
spatel wrote:
> Might as well create this as "SGT X, -1" to save a step since the "SGE" will get canonicalized to that form.
Will do.


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

https://reviews.llvm.org/D109151