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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked 5 inline comments as done.
cameron.mcinally added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9652
+  case ISD::VECREDUCE_SMAX: {
+    bool OverrideNEON = SrcVT.getVectorElementType() == MVT::i64;
+    if (useSVEForFixedLengthVectorVT(SrcVT, OverrideNEON))
----------------
paulwalker-arm wrote:
> 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?
Ok, I'll play around with it in the next patch.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16004
   if (ResVT != ScalarOp.getValueType())
-    Res = DAG.getNode(ISD::TRUNCATE, DL, ScalarOp.getValueType(), Res);
+    Res = DAG.getAnyExtOrTrunc(Res, DL, ScalarOp.getValueType());
 
----------------
paulwalker-arm wrote:
> paulwalker-arm wrote:
> > 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.
> 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.
Yeah, that struck me as strange too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88259