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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Thanks, looks ok, other than repetition..



================
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:
> > 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.



================
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:
> 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.


================
Comment at: llvm/test/CodeGen/AArch64/unfold-masked-merge-scalar-variablemask.ll:358
 }
 ; This is not a canonical form. Testing for completeness only.
 define i32 @out_constant_varx_mone_invmask(i32 %x, i32 %y, i32 %mask) {
----------------
(good thing i wrote all these then-pointless redundant tests i guess)


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

https://reviews.llvm.org/D59174