[PATCH] D12638: Improve ISel using across lane min/max reduction

[James Molloy via llvm-commits]

jmolloy accepted this revision.
jmolloy added a comment.

Hi,

This generally looks fine, I have a few specific comments and it looks like the testcases could probably do with some simplification (anonymizing of the long names?)

Apart from that LGTM.

Cheers,

James


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8612
@@ -8644,4 +8611,3 @@
       // Try to swap the 1st and 2nd operand as add is commutative.
-      CurOp = PreOp.getOperand(1);
-      Shuffle = PreOp.getOperand(0);
-      if (Shuffle.getOpcode() != ISD::VECTOR_SHUFFLE)
+      if (Op == ISD::ADD) {
+        CurOp = PreOp.getOperand(1);
----------------
Any reduction should be commutative, right? surely at least SMAX and UMAX should be? Not just ADD?

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8617
@@ -8648,1 +8616,3 @@
+          return SDValue();
+      } else
         return SDValue();
----------------
Braces around the else

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8622
@@ +8621,3 @@
+    // Check if the input vector is fed by the operator we want to handle,
+    // except the last step; the very first input vector is not necessary
+    // the same operator we are handling.
----------------
necessarily


http://reviews.llvm.org/D12638