[PATCH] D146404: Improve min/max vector reductions on arm

[Caleb Zulawski via Phabricator via llvm-commits]

calebzulawski marked 7 inline comments as done.
calebzulawski added a comment.

Thanks for the great review! I haven't touched codegen before, this helped quite a bit.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10288
 
+static SDValue LowerVecReduceMinMax(SDValue Op, SelectionDAG &DAG,
+                                    const ARMSubtarget *ST) {
----------------
dmgreen wrote:
> Is there a missing chunk where this gets called from LowerOperation?
Whoops, something was a little wonky with the last patch


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10335
+  if (EltVT != Op->getValueType(0))
+    Res = DAG.getNode(ISD::ANY_EXTEND, dl, Op->getValueType(0), Res);
+  return Res;
----------------
dmgreen wrote:
> Should this do SIGN_EXTEND or ZERO_EXTEND, depending on the sign of the min/max?
Yes, I think so. I copied it from the MVE vector reduce, but maybe that's doing something different.


================
Comment at: llvm/test/CodeGen/ARM/vecreduce-minmax.ll:4
+
+define i8 @test_umin_v8i8(<8 x i8> %x) {
+; CHECK-LABEL: test_umin_v8i8:
----------------
dmgreen wrote:
> Can you add some tests for <16 x i8> and other 128bit vector types. They will be quite common.
Aha, this also had the desired effect on much longer vectors, since they appear to break into 128-bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146404