[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 binop-and-shifts.ll reverted to the baseline checks.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:453-467
+  // (Binop1 (Binop2 (logic_shift X, C), C1), (logic_shift Y, C))
+  //   IFF
+  //    1) the logic_shifts match
+  //    2) either both binops are binops and one is `and` or
+  //       BinOp1 is `and`
+  //       (logic_shift (inv_logic_shift C1, C), C) == C1 or
+  //
----------------
I don't think listing the preconditions for the transform is particularly useful in the API docs, especially if they are this convoluted.

Alternatively, maybe move the comment into the source file.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:811-818
+    if (I.getOpcode() == Instruction::And) {
+      // Pass
+    }
+    // For all other possible transfers we need complete distributable
+    // binop/shift (anything but `add` + `lshr`).
+    else if (!IsCompletelyDistributable(I.getOpcode(), BinOpc, ShOpc)) {
+      return nullptr;
----------------



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:819-833
+    // If BinOp2 is `and`, and mask works (this only really helps for non-splat
+    // vecs, otherwise the mask will be simplified and the following check will
+    // handle it).
+    else if (BinOpc == Instruction::And) {
+      // Pass
+    }
+    // Otherwise, need mask that meets the below requirement.
----------------
Or alternative extract all the checks into a function returning bool, but unnecessary `// Pass` please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568