[PATCH] D157290: [InstCombine] Fold (-a >> b) and/or/xor (~a >> b) into (-a and/or/xor ~a) >> b

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

As far as I can tell the `sub 0, x` is irrelevant for the transform. Can you please remove it from the proofs (and tests)?



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:757
+//   IFF
+//   1) the arithmetic_shift match
+//   1) Binop1 is bitwise logical operator `and`, `or` or `xor`
----------------
There's just one kind of arithmetic shift, so it can't not match.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:853
+      if (Instruction::isBitwiseLogicOp(I.getOpcode()) &&
+          BinOpc == Instruction::Xor && CMask && CMask->isAllOnesValue()) {
+        Value *NotX = Builder.CreateBinOp(
----------------
Maybe `match(Mask, m_AllOnes())` and leave CMask alone? This will also match splat vectors with undef values, so we'd need a test for that.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:855
+        Value *NotX = Builder.CreateBinOp(
+            static_cast<Instruction::BinaryOps>(BinOpc), X, Mask);
+        Value *NewBinOp = Builder.CreateBinOp(
----------------
Might as well `CreateNot`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:857
+        Value *NewBinOp = Builder.CreateBinOp(
+            static_cast<Instruction::BinaryOps>(I.getOpcode()), Y, NotX);
+        return BinaryOperator::Create(
----------------
Cast shouldn't be needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157290