[PATCH] D88259: [SVE] Lower fixed length VECREDUCE_[SMAX|SMIN] to Scalable

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

Other than a quibble related to singleton DAG checks this patch LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9652
+  case ISD::VECREDUCE_SMAX: {
+    bool OverrideNEON = SrcVT.getVectorElementType() == MVT::i64;
+    if (useSVEForFixedLengthVectorVT(SrcVT, OverrideNEON))
----------------
We can see how it plays out but I imagine this will be needed for the unsigned equivalents and perhaps other reductions so perhaps worth pulling out of the switch to reduce the clutter?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-reduce.ll:225
 
-; Nothing to do for 64-bit vectors..
+; Nothing to do for 64-bit vectors.
 define i64 @uaddv_v1i64(<1 x i64> %a) #0 {
----------------
To be more accurate this is "Nothing to do for single element vectors."


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-reduce.ll:316
+; VBITS_GE_256: ptrue [[PG:p[0-9]+]].b, vl32
+; VBITS_GE_256-DAG: ld1b { [[OP:z[0-9]+]].b }, [[PG]]/z, [x0]
+; VBITS_GE_256-NEXT: smaxv b[[REDUCE:[0-9]+]], [[PG]], [[OP]].b
----------------
Sorry I missed this in the original patch but I think these singleton DAGs should be NEXT?  I suspect it's just a legacy from copying an existing two operand test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88259