Page MenuHomePhabricator

[SVE] Lower fixed length VECREDUCE_[SMAX|SMIN] to Scalable
ClosedPublic

Authored by cameron.mcinally on Sep 24 2020, 1:13 PM.

Details

Summary

This patch is pretty similar to the VECREDUCE_ADD patch, with some minor tweaks.

  1. Results from the AArch64ISD::[SMAX|SMIN]V_PRED return element sized results. This requires an ANY_EXTEND for results < 32-bits, since Legalization promotes those results. (*Unless I misunderstood something*)
  1. There is no NEON i64 vector support for SMAXV|SMINV, so use SVE for those.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
cameron.mcinally requested review of this revision.Sep 24 2020, 1:13 PM
paulwalker-arm added inline comments.Sep 24 2020, 4:05 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16021

I don't think an any extent is correct here as I'd expect SMINV to return a signed result and UMINV an unsigned one.

paulwalker-arm added inline comments.Sep 24 2020, 6:21 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16021

Please ignore my previous comment, I can see from the node's description is says However, the reduction is performed using the vector element type and the value in the top bits is unspecified so an any extent is exactly right.

paulwalker-arm accepted this revision.Sep 25 2020, 2:51 AM

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

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9669

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?

llvm/test/CodeGen/AArch64/sve-fixed-length-int-reduce.ll
225

To be more accurate this is "Nothing to do for single element vectors."

316

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.

This revision is now accepted and ready to land.Sep 25 2020, 2:51 AM
cameron.mcinally marked 5 inline comments as done.Sep 25 2020, 7:33 AM
cameron.mcinally added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9669

Ok, I'll play around with it in the next patch.

16021

Yeah, that struck me as strange too.

This revision was automatically updated to reflect the committed changes.
cameron.mcinally marked 2 inline comments as done.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1227

I accidentally committed these UMAX/UMIN changes. They have been removed with 9a4767411e89d35e55074e8783b909d0e8c6b2df.