[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 marked an inline comment as done.
goldstein.w.n 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:
> goldstein.w.n wrote:
> > pengfei wrote:
> > > 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?
> > > 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.
> > 
> > As simon suggested, split the two implementations. Think its a lot more readable now.
> > > 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.
> 
> I see. I think this is b.c of the reordering `(and (shl X..)) -> (shl (and X..))` when `X` persists (then we need a `mov`).
> We could prevent it with a guard in `combineAndWithLogicalShift` that only did the transform if this was the last use of `X` the shiftop was `shl 1/2/3` (can become lea). Is there a way to query "isLastUse(X)" for an `SDValue` or do we just need to do `hasOneUse`?
> 
> Also note other places in the patch get the inverse behavior i.e `llvm/test/CodeGen/X86/combine-rotates.ll:rotl_merge_i5`.
> 
> 
> > Is the TODO in the file to address this problem?
> 
> Not directly, no. Maybe by accident.
> > 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?

Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141653