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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic 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)) &&
----------------
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.


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

https://reviews.llvm.org/D59174