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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs marked 2 inline comments as done.
samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5168
+           CC == ISD::SETGT)
+    Opcode = ARMISD::VMAXVs;
+
----------------
dmgreen wrote:
> else return false;
> Then it doesn't need the extra indenting.
Thumbs up


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5186
+    // so check the first operand in those cases
+    switch (LHS->getOpcode()) {
+    case ISD::AND:
----------------
dmgreen wrote:
> This looks like it need to be a bit stricter still. The AND needs to go with a umin/umax, and the sign extend with smin/smax (they shouldn't be the other way around). The AND needs a mask of the right size (it'll be 255 for i8 for example, which is the same as a "zero extend inreg"). The sign extend will have a type as the second argument, which should be the same as the vecreduce's scalar type.
> 
> Some of this might be difficult to get to come up in practice, but we should make sure it won't be subtly wrong and cause bugs.
I thought about those two but hoped that the IR type system would make those situations impossible. Will add these, thanks.


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

https://reviews.llvm.org/D87836