[PATCH] D152568: [InstCombine] Transform `(binop1 (binop2 (lshift X,Amt),Mask),(lshift Y,Amt))`

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

It looks like there are some more tests that need to be updated.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:771
+    // LHS and RHS need same shift opcode
+    ShOpc = IY->getOpcode();
+    if (ShOpc != IX->getOpcode())
----------------
Here and elsewhere: Keep variable scope minimal.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:775
+
+    // Make sure binop is real instruction and not ContstantExpr
+    auto *BO2 = dyn_cast<Instruction>(I.getOperand(1 - ShOpnum));
----------------
ConstantExpr


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:785
+
+    // If BinOp1 == BinOp2 and its bitwise or shl with add, then just
+    // reassosiate to drop the shift irrelivant of constants.
----------------
it's


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:786
+    // If BinOp1 == BinOp2 and its bitwise or shl with add, then just
+    // reassosiate to drop the shift irrelivant of constants.
+    if (BinOpc == I.getOpcode() &&
----------------
irrelevant


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:795
+
+    // Otherwise we can only reassosiate by constant shifting the mask, so
+    // ensure we have constants.
----------------
reassociate

Though this is really more "distribute" than reassociate.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:812
+      // Pass
+    }
+    // If both BinOp1 and BinOp2 are bitwise ops and:
----------------
Does this case trigger for anything other than non-splat vectors? If not, please remove it. (Generally, don't add non-splat vector support it it requires //any// additional complexity.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:820
+                               ConstantExpr::get(InvShOpc, CMask, CShift),
+                               CShift) == CMask) {
+      // Pass
----------------
Isn't this also fine for add/shl, as in the case above? https://alive2.llvm.org/ce/z/ztthn5


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568