[PATCH] D93050: [SVE][CodeGen] Lower scalable floating-point vector reductions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16820
+  if (SrcVT.isScalableVector() && SrcVT.isFloatingPoint())
+    RdxVT = SrcVT;
 
----------------
paulwalker-arm wrote:
> kmclaughlin wrote:
> > cameron.mcinally wrote:
> > > Nit: We could conditionally avoid the call to `getPackedSVEVectorVT(...)` by inverting this condition and initial value of RdxVT. Something like:
> > > 
> > > ```
> > >   EVT RdxVT = SrcVT; 
> > >   if (SrcVT.isFixedVector() || SrcVT.isInteger())
> > >     RdxVT = getPackedSVEVectorVT(ResVT);
> > > ```
> > > 
> > > I also wonder if we need the `isFloatingPoint()/isInteger()` check at all. Is just `isScalableVector()` enough?
> > Hi @cameron.mcinally , I've inverted the condition as you suggested here and removed the isFloatingPoint() check. The reason I included the additional check was for UADDV, where the result type should always be `nxv2i64`. Though as it's the only case for scalable vectors that needs to use `getPackedSVEVectorVT`, I've added a check for the UADDV_PRED opcode here instead.
> I've nothing against the current approach but there is an argument for unpacked scalable vectors to be treated more like how fixed length vectors are handled.  For example we could change the current `if (useSVEForFixedLengthVectorVT` block to:
> ```
> if (SrcVT.isFixedLengthVector()) {
>     EVT ContainerVT = getContainerForFixedLengthVector(DAG, SrcVT);
>     VecOp = convertToScalableVector(DAG, ContainerVT, VecOp);
> } else if (!isPackedVectorType(SrcVT)) {
>     EVT ContainerVT = getPackedSVEVectorVT(DAG, SrcVT);
>     VecOp = DAG.getNode(AArch64ISD::REINTERPRET_CAST, dl, ContainerVT, VecOp);
> }
> ```
> That way the remainder of the function remains as is and you don't need to change SVEInstrFormats.td because these nodes no longer have to worry about unpacked types. The same argument applies to the ordered reductions as well.
> 
> As I say, I'm happy either way, so just throwing it out there.  What do you think?
Actually I retract this.  getPredicateForVector doesn't quite work the way we'll need it to.  It's something to consider for the future, perhaps as part of a larger effort to remove the need to support unpacked vector types during isel.


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

https://reviews.llvm.org/D93050