[PATCH] D74032: [DAGCombine][ARM] Combine pattern for REV16

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5668-5671
+  if (!((Mask1->getAPIntValue() == 0xff00ff00 &&
+         Mask2->getAPIntValue() == 0x00ff00ff) ||
+        (Mask1->getAPIntValue() == 0x00ff00ff &&
+         Mask2->getAPIntValue() == 0xff00ff00)))
----------------
This check works, but it makes me nervous because I think it requires demanded bits to alter the code for correctness.
Take this example where the masks are paired with the wrong shift op (and please add a test like this to the patch):

```
define i32 @not_rev16(i32 %a) {
    %l8 = shl i32 %a, 8
    %r8 = lshr i32 %a, 8
    %mask_r8 = and i32 %r8, 4278255360
    %mask_l8 = and i32 %l8, 16711935
    %tmp = or i32 %mask_r8, %mask_l8
    ret i32 %tmp
}

```

To be safe, I think we should enforce that the masks and shifts are paired correctly.
So we could do something like:

```
// Canonicalize mask ops to ensure that shift-left operand is on the left.
if (Mask2 == 0xff00ff00) {
  std::swap(N0, N1);
  std::swap(Mask1, Mask2)
}
```

or maybe better - go back to the earlier rev of this patch and call this function with the operands reversed:

```
  if (SDValue BSwap = matchBSwapHWordOrAndAnd(TLI, DAG, N, N0, N1, VT,
                                              getShiftAmountTy(VT)))
    return BSwap;
  // Try again with commuted operands.
  if (SDValue BSwap = matchBSwapHWordOrAndAnd(TLI, DAG, N, N1, N0, VT,
                                              getShiftAmountTy(VT)))
    return BSwap;

```


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

https://reviews.llvm.org/D74032