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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5188
 
+  if (Subtarget->hasMVEIntegerOps()) {
+    unsigned int Opcode = 0;
----------------
dmgreen wrote:
> 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.
That's much cleaner! Done.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5220-5222
+        SDValue tmp = Left;
+        Left = Right;
+        Right = tmp;
----------------
dmgreen wrote:
> This could use std::swap?
Ah I didn't know that existed. Done.


================
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
----------------
dmgreen wrote:
> 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.
Thanks. I've added explicit checks for the LHS opcode and have made the lowering use the LHS instead of the TrueVal as the scalar so that we know the top 24 bits aren't set.


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

https://reviews.llvm.org/D87836