[PATCH] D87836: [ARM]Fold select_cc(vecreduce_[u|s][min|max], x) into VMINV or VMAXV

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5188
 
+  if (Subtarget->hasMVEIntegerOps()) {
+    unsigned int Opcode = 0;
----------------
It's might be worth pulling this out into a separate function, similar to isLowerSaturatingConditional or something like PerformSplittingToNarrowingStores where we return the new SDValue.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5220-5222
+        SDValue tmp = Left;
+        Left = Right;
+        Right = tmp;
----------------
This could use std::swap?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5231-5233
+      // the scalar on the left. Sometimes the left and/or right side wrap the
+      // scalar and/or reduction in another operation, like an AND or
+      // sign-extension, so check the first operand as well
----------------
I think we might need to check these things, otherwise we might be matching things incorrectly.

I believe the exact semantics of a vminv.s8 are that it reads the bottom 8 bits of Rn to do the final scalar min. So a 32bit min could actually produce a different value. We should make sure we are getting this correct too.


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

https://reviews.llvm.org/D87836