[PATCH] D141179: [X86]] Search through commutable operators for BMI patterns (BLSI, BLSR, BLSMSK)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48801
 
+static SDValue GetBMIMatchingOp(unsigned OPC, SelectionDAG &DAG,
+                                SDValue OpMustEq, SDValue Op, unsigned Depth) {
----------------
Do not use all caps for `OPC`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48808
+  // Only can do this re-ordering if op has one use, op has the same type
+  if (!Op.hasOneUse() || Op.getValueType() != OpMustEq.getValueType())
+    return SDValue();
----------------
If you only look through binops without going through bitcasts, extends, or truncates, you can assume the types will always match.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48828
+      // BLSI: (and x, (sub 0, x))
+      auto *C = dyn_cast<ConstantSDNode>(Op.getOperand(0));
+      if (C && C->isZero() && Op.getOperand(1) == OpMustEq) {
----------------
You can use `isNullConstant(Op.getOperand(0)` here.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48837
+    auto *C = dyn_cast<ConstantSDNode>(Op.getOperand(1));
+    if (C && C->isOne() && Op.getOperand(0) == OpMustEq) {
+      return DAG.getNode(OPC, DL, Op.getValueType(), OpMustEq, Op);
----------------
isOneConstant(Op.getOperand(1))


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48846
+      auto *C = dyn_cast<ConstantSDNode>(Op.getOperand(OpIdx));
+      if (C && C->isAllOnes() && Op.getOperand(1 - OpIdx) == OpMustEq) {
+        return DAG.getNode(OPC, DL, Op.getValueType(), OpMustEq, Op);
----------------
isAllOnesConstant(Op.getOperand(OpIdx)


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48854
+
+static SDValue CombineBMILogicOp(SDNode *N, SelectionDAG &DAG,
+                                 const X86Subtarget &Subtarget) {
----------------
Lower case start of function name


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48858
+  // Make sure this node is a candidate for BMI instructions.
+  if (!Subtarget.hasBMI() || !VT.isScalarInteger() || VT.getSizeInBits() < 32 ||
+      VT.getSizeInBits() > 64)
----------------
Just check VT != MVT::i32 && VT != MVT::i64 unless you're trying to support i33-i63?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48865
+  // Try and match LHS and RHS.
+  for (unsigned OpIdx = 0; OpIdx < 2; ++OpIdx) {
+    if (SDValue OpMatch =
----------------
You can drop all the curly braces here.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49096
 
+  if(SDValue R = CombineBMILogicOp(N, DAG, Subtarget))
+      return R;
----------------
Space after `if`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:49097
+  if(SDValue R = CombineBMILogicOp(N, DAG, Subtarget))
+      return R;
+
----------------
Over indented


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:52013
 
+  if(SDValue R = CombineBMILogicOp(N, DAG, Subtarget))
+      return R;
----------------
Space after `if`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:52014
+  if(SDValue R = CombineBMILogicOp(N, DAG, Subtarget))
+      return R;
+
----------------
Over indented


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141179