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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n 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);
----------------
pengfei wrote:
> So it is only profitable when `MASK` is a constant? Why don't use `c3` in the comments?
> So it is only profitable when `MASK` is a constant? Why don't use `c3` in the comments?




================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:47479
+static SDValue
+combineLogicalShiftWithAnd(SDNode *N, SelectionDAG &DAG,
+                           TargetLowering::DAGCombinerInfo &DCI) {
----------------
pengfei wrote:
> 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.
> 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 think the bitreverse.ll regressions have been fixed (that was the point of the new patterns in `DAGCombiner.cpp`).

> I may not try to understand the code here, leave it for other reviewers.

Okay, I will split into 2-functions as Simon suggested which I think will decrease the complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141653