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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D74032#1876021 <https://reviews.llvm.org/D74032#1876021>, @SjoerdMeijer wrote:

> Many thanks again for the feedback! Really liked that suggestion: so now calling the same function twice but with the operands swapped to check the commuted case. Have also added the suggested test case.


There's 1 more concern for this transform - do we need to limit it when the intermediate values have other uses? This will be interesting because the answer may be different for different targets. Ie, for ARM/Thumb/AArch64, we're able to reduce the whole sequence to a single rev16, so it's always a good transform. But for x86, we're going to need a bswap and ror.

So we need to:

1. Add a test like this:

  define i32 @extra_maskop_uses2(i32 %a) {
      %l8 = shl i32 %a, 8
      %r8 = lshr i32 %a, 8
      %mask_l8 = and i32 %l8, 4278255360
      %mask_r8 = and i32 %r8, 16711935
      %or = or i32 %mask_r8, %mask_l8
      %mul = mul i32 %mask_r8, %mask_l8   ; use the mask ops for some other reason 
      %r = mul i32 %mul, %or              ; and use that result for some other reason
      ret i32 %r
  }

2. Copy that test (maybe the whole test file) over to llvm/tests/CodeGen/X86 and generate CHECK lines with utils/update_llc_test_checks.py.
3. Possibly add some 'hasOneUse()' logic to the code to avoid regressions.

Please push the test changes to trunk before updating in this review (no need for pre-commit review unless you have questions/concerns).



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5664-5665
+    return SDValue();
+  ConstantSDNode *Mask1 = isConstOrConstSplat(N0.getOperand(1));
+  ConstantSDNode *Mask2 = isConstOrConstSplat(N1.getOperand(1));
+  if (!Mask1 || !Mask2)
----------------
Nit: the input variables are "0" and "1", but this shifts the indexing to "1" and "2". It would be better to make this consistent (same for the "Shift" variables below this). 

Most of the code in DAGCombiner uses "0"-based indexing.


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

https://reviews.llvm.org/D74032