This is an archive of the discontinued LLVM Phabricator instance.

[SVE][CodeGen] Lower scalable floating-point vector reductions
ClosedPublic

Authored by kmclaughlin on Dec 10 2020, 9:32 AM.

Details

Summary

Changes in this patch:

  • Minor changes to the LowerVECREDUCE_SEQ_FADD function added by @cameron.mcinally to also work for scalable types
  • Added TableGen patterns for FP reductions with unpacked types (nxv2f16, nxv4f16 & nxv2f32)
  • Asserts added to expandFMINNUM_FMAXNUM & expandVecReduceSeq for scalable types

Diff Detail

Event Timeline

kmclaughlin created this revision.Dec 10 2020, 9:32 AM
kmclaughlin requested review of this revision.Dec 10 2020, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 9:32 AM

LGTM with one nit below...

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

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?

kmclaughlin marked an inline comment as done.
  • Reordered the condition added to LowerReductionToSVE which sets RdxVT
kmclaughlin marked an inline comment as not done.Dec 11 2020, 4:26 AM
kmclaughlin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16842

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.

paulwalker-arm added inline comments.Dec 11 2020, 5:06 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16773

By getting this far we've already committed to using SVE so can this just be SrcVT.isFixedLengthVector()?

16842

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?

paulwalker-arm added inline comments.Dec 11 2020, 5:55 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16842

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.

This revision is now accepted and ready to land.Dec 11 2020, 7:17 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.

Thanks @cameron.mcinally & @paulwalker-arm for reviewing this patch!