Page MenuHomePhabricator

[AArch64][SVE] Add support for fixed length MSCATTER/MGATHER
ClosedPublic

Authored by bsmith on Jun 14 2021, 4:39 AM.

Details

Summary

Since gather lowering can now lower to nodes that may need expansion via
the vector legalizer, do MGATHER lowering via vector legalizer.

Additionally, as part of adding passthru support for fixed typed
gathers, fix passthru support for scalable types.

Depends on D104910

Diff Detail

Event Timeline

bsmith created this revision.Jun 14 2021, 4:39 AM
bsmith requested review of this revision.Jun 14 2021, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2021, 4:39 AM
Matt added a subscriber: Matt.Jun 14 2021, 12:22 PM
paulwalker-arm added inline comments.Jun 15 2021, 4:57 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
18105–18106

I guess you've moved these lowering functions because of the placement of the fixed length utility functions. I guess you could move the utility functions but I'm wondering if you can just add forward declarations to the top somewhere?

bsmith updated this revision to Diff 352430.Jun 16 2021, 7:09 AM
bsmith marked an inline comment as done.
  • Restore file position of LowerMGATHER/LowerMSCATTER and instead add forward declarations of SVE helpers.
  • Fix clang format issues

Mostly nits -- looks like it needs a rebase.

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

Is it intended to address this in the near future?

Nit: I initially read 'the code should be moved, once <condition>', but I think you mean, 'the code should be moved, so that it executes after <condition>'.

4333

Nit: } else if and remove a level of indent.

4340

I wonder if the undef-or-zero logic can be cleanly combined into a function with MLOAD. But maybe not. I wouldn't wonder for too long.

Thought (1): is the PassThru value actually needed for the Ops, or could it use a constant there? My thinking is that the underlying operation is invariant of its PassThru operand (hence the need for the select).

If (1) is right, then could you use an empty SDValue, which gets assigned to the thing to be 'selected', and the logic becomes if (PassThru) { insert select }.

I think this is a matter of taste so feel at liberty to ignore the suggestion. If you go for it please update MLOAD.

4448

Pondering: Why are these two isFixedLengthVector queries needed? I read: you have a fixed-vector operation, and it has scalable operands? Can that arise? How so?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
246–252

This was changed in D101834 so it looks like you need a rebase since the MLOAD work.

255

Nit: Potentially confusing comment about the mechanism of the following block, because it's written in actively: "this block avoids scalarization", but actually the only thing the block can do is return false, which causes scalarization. Suggested form: "Fixed-length vectors do not use masked gather/scatter unless ..."

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

Is it intended to address this in the near future?

I've thought about/discussed this am happy with having the FIXME here.

bsmith updated this revision to Diff 352764.Jun 17 2021, 9:41 AM
bsmith marked 6 inline comments as done.
  • Simplify passthru for gathers
  • Add tests for type legalization
  • Clarify some comments
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4340

This makes sense for MGATHER since the passthru value is totally ignored, however for masked_load we don't have target specific isd nodes, hence the passthru value makes it down to selection.

4448

selectGatherScatterAddrMode can swap the BasePtr/Index operands, one of which is a scalar, the other a vector. Hence here we don't know which is which, so we check them both.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
246–252

It was isLegalMaskedLoadStore that was changed in D101834, not this function.

david-arm added inline comments.Jun 24 2021, 7:31 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1215–1216

Does the comment need updating to reflect gathers/scatters too?

1215–1216

Not sure if it matters, but isLegalMaskedGatherScatter explicitly excludes MVT::v1f64 as it has < 2 elements.

1216

I realise this has nothing to do with your change, but it looks really odd that we have a nested loop here where we override the VT from the outer loop. Essentially, we're registering the exact same custom operation 4 times in a row! Might be worth fixing at some point.

4324

Is it worth asserting here that Subtarget->useSVEForFixedLengthVectors() returns true?

16480

Is there to do this in a separate patch? I think it would be better if possible - I imagine it works as a standalone change where it tidies up the masked load/store tests.

bsmith updated this revision to Diff 354477.Jun 25 2021, 5:39 AM
bsmith marked 4 inline comments as done.
bsmith edited the summary of this revision. (Show Details)
bsmith removed a reviewer: joechrisellis.
  • Split out SETCC_MERGE_ZERO dag combine into another patch
  • Fixed incorrectly nested loops during setOperationAction's
  • Add extra asserts in Lowering functions
  • Minor comment corrections
  • Minor test changes due to other commits
peterwaller-arm accepted this revision.Tue, Jun 29, 10:20 AM
This revision is now accepted and ready to land.Tue, Jun 29, 10:20 AM

Could you fix all the formatting issues before submitting? Thanks!

This revision was landed with ongoing or failed builds.Thu, Jul 1, 4:15 AM
This revision was automatically updated to reflect the committed changes.