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

I think you fixed combine-bitreverse.ll rather than bitreverse.ll
The problem in bitreverse.ll is 2 lea is replaced by 2 mov + and + shl. I think it is a minor regression due to the increased 2 mov, so I didn't insist on that.
Is the TODO in the file to address this problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141653