[PATCH] D141653: [X86] Improve instruction ordering of constant `srl/shl` with `and` to get better and-masks

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9310
+    if (N0.getOpcode() == ISD::AND && N0.hasOneUse()) {
+      if (dyn_cast<ConstantSDNode>(N0.getOperand(1))) {
+        SDValue N00 = N0.getOperand(0);
----------------
So it is only profitable when `MASK` is a constant? Why don't use `c3` in the comments?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9318
+                                        /*AllowTypeMismatch*/ true)) {
+            SDValue N001 = DAG.getZExtOrTrunc(N00.getOperand(1), DL, ShiftVT);
+            SDValue Diff = DAG.getNode(ISD::SUB, DL, ShiftVT, N001, N1);
----------------
Duplicated define.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47459-47460
+
+    if (Opc == ISD::TRUNCATE || Opc == ISD::ZERO_EXTEND ||
+        Opc == ISD::SIGN_EXTEND || Opc == ISD::ANY_EXTEND ||
+        Opc == ISD::BITCAST) {
----------------
You could use `ISD::isExtOpcode(Opc)`.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47479
+static SDValue
+combineLogicalShiftWithAnd(SDNode *N, SelectionDAG &DAG,
+                           TargetLowering::DAGCombinerInfo &DCI) {
----------------
I'd say the implementation is too complicated to me to understand. And the overall improvement doesn't look worth the complexity. Not to mention there's still regression in bitreverse.ll
I may not try to understand the code here, leave it for other reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141653