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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Sounds like a nice idea. This may want to custom lower 128bit vectors too, to turn them into `vpmin.u8 lowhalf, highhalf` as a first step.

Can you upload with full context, as per https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch. It can make them easier to review.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:250
+
+  for (auto VT : IntTypes) {
+    setOperationAction(ISD::VECREDUCE_SMAX, VT, Custom);
----------------
I think I would move this into one of the `if (Subtarget->hasNEON()) {` blocks in ARMTargetLowering::ARMTargetLowering. Maybe near all the legal extload setLoadExtAction's. At least that seems to be how things have worked so far.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10288
 
+static SDValue LowerVecReduceMinMax(SDValue Op, SelectionDAG &DAG,
+                                    const ARMSubtarget *ST) {
----------------
Is there a missing chunk where this gets called from LowerOperation?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10290
+                                    const ARMSubtarget *ST) {
+  if (ST->hasMVEIntegerOps() || !ST->hasNEON())
+    return SDValue();
----------------
This can just check for `if (!ST->hasNEON())` - Neon and MVE are mutually exclusive.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10322
+
+  // use pairwise reductions until one lane remains
+  while (NumActiveLanes > 1) {
----------------
Capitalize the U in use.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:10334
+  // Result type may be wider than element type.
+  if (EltVT != Op->getValueType(0))
+    Res = DAG.getNode(ISD::ANY_EXTEND, dl, Op->getValueType(0), Res);
----------------
Op.getValueType()


================
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;
----------------
Should this do SIGN_EXTEND or ZERO_EXTEND, depending on the sign of the min/max?


================
Comment at: llvm/test/CodeGen/ARM/vecreduce-minmax.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+neon -verify-machineinstrs | FileCheck %s --check-prefix=CHECK
+
----------------
Adding -float-abi=hard might simplify the tests a little. Maybe use `-mtriple=armv7-none-eabi` for a newer target triple too? And the `--check-prefix=CHECK` is already the default and can be removed.


================
Comment at: llvm/test/CodeGen/ARM/vecreduce-minmax.ll:4
+
+define i8 @test_umin_v8i8(<8 x i8> %x) {
+; CHECK-LABEL: test_umin_v8i8:
----------------
Can you add some tests for <16 x i8> and other 128bit vector types. They will be quite common.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146404