[PATCH] D59174: [DAGCombine] Fold (x & ~y) | y patterns

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5429
 
+  // fold (or (and X, (xor Y, -1)), Y) -> (or X, Y)
+  if (N0.getOpcode() == ISD::AND && isBitwiseNot(N0.getOperand(1)) &&
----------------
nikic wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > nikic wrote:
> > > > lebedev.ri wrote:
> > > > > There are 4 variations of this pattern, both `or` and `and` are commutative.
> > > > Right you are. I've added these now, which resulted in a few more changes to the variablemask tests. I was under the mistaken impression that `(and X, (xor Y, -1))` is a canonicalized and-not pattern.
> > > That is indeed a very subtle (and costly!) thing.
> > > `(and X, (xor Y, -1))` will be in that order (well, some deterministic order).
> > > But what if `X` and `Y` aren't arguments, but some values (instructions)?
> > > Then they will also affect the weights and thus ordering.
> > > 
> > Hmm, nobody likes that there is no pattern matching in back end :/
> > But this duplication isn't too nice too.
> > Can you try this instead:
> > ```
> > // Fold all 4 commutative variations of    or (and (xor Y, -1), X), Y
> > // to                                      or X, Y
> > static SDValue foldDegradedBitselect(SDNode *N, SelectionDAG &DAG) {
> >   assert(N->getOpcode() == ISD::OR && "outermost node should be 'or'.");
> > 
> >   SDValue X, Y;
> >   auto outerMatcher = [N, &X, &Y](bool swap) {
> >     SDValue And = N->getOperand(0);
> >     Y = N->getOperand(1);
> > 
> >     if (And.getOpcode() != ISD::AND || swap)
> >       std::swap(And, Y);
> >     if (And.getOpcode() != ISD::AND)
> >       return false;
> > 
> >     auto innerMatcher = [And, Y, &X](bool swap) {
> >       assert(And.getOpcode() == ISD::AND);
> > 
> >       SDValue NotY = And.getOperand(0);
> >       X = And.getOperand(1);
> > 
> >       if (!isBitwiseNot(NotY) || swap)
> >         std::swap(NotY, X);
> >       if (!isBitwiseNot(NotY))
> >         return false;
> > 
> >       SDValue Y2 = NotY.getOperand(0);
> > 
> >       return Y == Y2;
> >     };
> > 
> >     return innerMatcher(/*swap=*/false) || innerMatcher(/*swap=*/true);
> >   };
> > 
> >   if (!(outerMatcher(/*swap=*/false) || outerMatcher(/*swap=*/true)))
> >     return SDValue();
> > 
> >   return DAG.getNode(ISD::OR, SDLoc(Y), Y.getValueType(), X, Y);
> > }
> > ```
> > ?
> > 
> > If that doesn't work, at least factor out these 4 duplicate if's into a function.
> I've extracted a function that will be called with N0, N1 and N1, N0 to avoid repeating the outer pattern, and because this seems useful for potential future transforms as well.
> 
> This still repeats the and commutation, but I think the explicit repetition here is more obvious than the operand swapping variant.
Ok, in this case i guess that works too.


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

https://reviews.llvm.org/D59174