This is an archive of the discontinued LLVM Phabricator instance.

[llvm][sve] Lowering for VLS MLOAD/MSTORE
ClosedPublic

Authored by DavidTruby on May 4 2021, 7:31 AM.

Details

Summary

This adds custom lowering for the MLOAD and MSTORE ISD nodes when
passed fixed length vectors in SVE. This is done by converting the
vectors to VLA vectors and using the VLA code generation.

Fixed length extending loads and truncating stores currently produce
correct code, but do not use the built in extend/truncate in the
load and store instructions. This will be fixed in a future patch.

Diff Detail

Event Timeline

DavidTruby created this revision.May 4 2021, 7:31 AM
DavidTruby requested review of this revision.May 4 2021, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 7:31 AM

Disclosure: I had a hand in writing an early draft of a small part of this, so should definitely not be the only reviewer.

Early comments on first pass through.

I wonder for a moment: given that the "if condition"s are wrong in the LowerOperation switch -- and the tests didn't catch it means that we should have a test which would have caught it. Not sure what that would look like at the moment beyond it would need to be a test for non-scalable vector type.

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

Nit: Extraneous blank.

4555

Suggestion to avoid the cast.

Also, looks like you're using the "comma true" operator in an if, which I'm guessing is unintended?

4603

Comma true again.

17206

Nit: Extraneous blank

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
230

Could you please run clang-format on the patch?

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

nit: is MVT::v2f16 missing or is this type widened?

4603

Are masked SVE masked load/store instructions not always more efficient than scalarising?

17207

nit: It would be nice if this could be part of convertToScalableVector. That way, for any fixed-width vector we can ask for its scalable counterpart using the same interface.

17242

This may not be worth it, but just sharing my thoughts; when reading this code I kind of thought "Why not just call convertToScalableVector?". This would then end up as a INSERT_SUBVECTOR of a DUP, etc. But that could be simplified by a DAG combine which removes the INSERT_SUBVECTOR.

llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
31

Please use CHECK-NEXT for these, so that we can check there are no other instructions being emitted.

DavidTruby updated this revision to Diff 343014.May 5 2021, 5:31 AM

Fixed erroneous use of ", true" and ran clang-format

Also split the tests into separate files for mload and mstore

Matt added a subscriber: Matt.May 6 2021, 7:26 AM

Improved tests and removed unnecessary if statements

DavidTruby marked 4 inline comments as done.May 12 2021, 5:19 AM
DavidTruby added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1181

I believe v2f16 isn't a legal type here, so we don't want to do the custom lowering for it. We can just allow it to be handled as before.

4603

These if statements aren't necessary, as we won't select custom lowering here unless we already have a type that we want to use this lowering for. Consequently I've just removed them.

DavidTruby marked an inline comment as done.

Spelling and whitespace fixes

DavidTruby marked 2 inline comments as done.May 12 2021, 5:23 AM

Add guard against truncating stores and extending loads

bsmith added inline comments.May 13 2021, 7:03 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17223

Nit: Neon sized vectors come through here also

llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
3

Nit: VBITS_GE_256 is redundant (it's used in all the same places as CHECK), just use CHECK.

llvm/test/CodeGen/AArch64/sve-fixed-length-masked-stores.ll
30

Why is -NEXT missing from all of the fcmeq lines?

I see now that the loads are missing from the CHECK lines, they should probably be in for completeness. (Same for loads tests)

kmclaughlin added inline comments.May 13 2021, 10:32 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17217–17220

nit: can you just use return DAG.getNode(AArch64ISD::SETCC_MERGE_ZERO... here instead?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
231–232

nit: is it maybe worth merging this with the if (!ST->hasSVE()) condition above?

DavidTruby updated this revision to Diff 346131.EditedMay 18 2021, 4:48 AM

Update tests for review and fix erroneous if statement
Include suggestion of returning automatically rather than introducing a variable

DavidTruby added inline comments.May 18 2021, 4:51 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
231–232

I kept these conditions separate because they're logically different and the if statement gets difficult to grok in my opinion if they're combined

sdesmalen added inline comments.May 19 2021, 1:01 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1181

Fair enough. Would it be good to at least add a test for it?

17226

Are you planning to handle sign/zero-extending loads in a follow-up patch?

DavidTruby added inline comments.May 19 2021, 6:56 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1181

I can do, although it's not really testing anything this patch is doing: the <2 x half> seems to be canonicalised before we come through these code paths so I assume that's already tested elsewhere.
Let me know if you want an added test for completeness though :)

17218

Not sure how this got through, I'll re-format in the next revision or before pushing.

17226

Right now they work but perform the extension manually (as you can see in the tests)
The plan is to submit a follow up patch implementing custom lowering for them to use the built in extension in the load instructions

17242

Initially this is how I had tried to do it but I was getting selection failures because the types weren't quite right, so I decided to just manually create the correct vectors here

sdesmalen added inline comments.May 19 2021, 7:03 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1181

Let me know if you want an added test for completeness though :)

That would be good. I'm aware your patch isn't really doing anything specific for that type and I would suspect it all just works, but to confirm that I would have to download the patch, build it, and try it out, whereas it could also be confirmed (and guarded) by an extra test case.

Added tests for <2 x half>

sdesmalen accepted this revision.May 20 2021, 3:40 AM

LGTM, thanks for adding the test.

llvm/test/CodeGen/AArch64/sve-fixed-length-masked-loads.ll
35–36

nit:

fmov s[[V0:[0-9]+]], [[W0]]
mov v[[V0]].s[1], [[W1]]
43

nit:

mov [[V1]].h[1], [[W0]]
This revision is now accepted and ready to land.May 20 2021, 3:40 AM

Ensure v and s registers used are the same in <2 x half> tests

This revision was landed with ongoing or failed builds.May 20 2021, 4:06 AM
This revision was automatically updated to reflect the committed changes.