[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 accepted this revision.
pengfei added a comment.
This revision is now accepted and ready to land.

The new code is a bit easier to understand now, at least to me. The idea masks sense to me though I still think the logic is a bit verbose.
I'm not going to block this patch, but please wait some days for other reviewers.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9414
   return SDValue();
-}
+  }
 
----------------
Unintended change.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47663
+static bool isMaskAlreadyIdeal(unsigned DesiredMask, bool PreserveANDN,
+                               APInt const &AndMask) {
+  if (PreserveANDN && AndMask.getSignificantBits() <= 32)
----------------
I found we prefer to `const APInt`.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47674
+  } else if (AndMask.isMask()) {
+    unsigned MaskCnt = AndMask.getBitWidth() - AndMask.countLeadingZeros();
+    // Check if we already have an ideal (movl/movzwl/movzbl form) mask.
----------------
`countTrailingOnes`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47676
+    // Check if we already have an ideal (movl/movzwl/movzbl form) mask.
+    if (MaskCnt >= 8 && isPowerOf2_32(MaskCnt))
+      return true;
----------------
Should it just need to check `MaskCnt == 8/16/32`. Or even `AndMask == 0xff/0xffff/0xffffffff`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47706
+  if (NewAndMask.isMask()) {
+    unsigned NewMaskCnt = NewAndMask.countPopulation();
+    if (isPowerOf2_32(NewMaskCnt) && NewMaskCnt >= 8)
----------------
`countTrailingOnes`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47735
+
+  // Get the shift amount amount, only proceed if its constant.
+  auto *ShiftC = dyn_cast<ConstantSDNode>(N->getOperand(1));
----------------
Duplicated.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47735
+
+  // Get the shift amount amount, only proceed if its constant.
+  auto *ShiftC = dyn_cast<ConstantSDNode>(N->getOperand(1));
----------------
pengfei wrote:
> Duplicated.
it's


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47847
+
+  // Only proceed if shift-amount if constant.
+  auto *ShiftC = dyn_cast<ConstantSDNode>(ShiftOp->getOperand(1));
----------------
is


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:56681
 
+bool X86TargetLowering::isDesirableToCommuteWithShift(
+    const SDNode *N, CombineLevel Level) const {
----------------
Add a comment to explain this is for `combineLogicalShiftWithAnd`.
Why `combineAndWithLogicalShift` not need this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141653