[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:5654
+//   (rotr (bswap A), 16)
+static SDValue MatchBSwapHWordOrAndAnd(const TargetLowering &TLI,
+                                       SelectionDAG &DAG, SDNode *N, SDValue N0,
----------------
Use current formatting rules, so lowerCamel: "matchBSwap..."
(can fix the related function names as a preliminary step)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5657
+                                       SDValue N1, EVT VT, EVT ShiftAmountTy) {
+  assert(VT == MVT::i32 && "MatchBSwapHWordOrAndAnd: expecting i32");
+  if (!TLI.isOperationLegalOrCustom(ISD::ROTR, VT))
----------------
Also assert that N->getOpcode() == ISD::OR ?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5666-5668
+  if (Mask1->getAPIntValue() != 0xff00ff00 ||
+      Mask2->getAPIntValue() != 0x00ff00ff)
+    return SDValue();
----------------
This is too specific - what if the operands of the "or" are commuted?

This patch should have a test for that possibility:

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

```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5673-5674
+    return SDValue();
+  ConstantSDNode *Const1 = isConstOrConstSplat(Shift1.getOperand(1));
+  ConstantSDNode *Const2 = isConstOrConstSplat(Shift2.getOperand(1));
+  if (!Const1 || !Const2)
----------------
These could use more descriptive names: ShiftAmt{1/2}.
These are using isConstOrConstSplat(), but the 'and' mask operands are not. Change the above code to match this?


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

https://reviews.llvm.org/D74032