This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add new ld<n> intrinsics that return a struct of vscale types
ClosedPublic

Authored by bsmith on Oct 19 2021, 8:12 AM.

Details

Summary

This will allow us to reuse existing interleaved load logic in
lowerInterleavedLoad that exists for neon types, but for SVE fixed
types.

The goal eventually will be to replace the existing ld<n> intriniscs
with these, once a migration path has been sorted out.

Diff Detail

Event Timeline

bsmith created this revision.Oct 19 2021, 8:12 AM
bsmith requested review of this revision.Oct 19 2021, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 8:12 AM
Matt added a subscriber: Matt.Oct 19 2021, 9:43 AM

Hi @bsmith
I was not added to the review so you can ignore my comments.
But if you want to increase a fellow contributor knowledge in LLVM I posted some questions.

llvm/include/llvm/IR/IntrinsicsAArch64.td
977

Should this be align? Like it is in class AdvSIMD_4Vec_PredStore_Intrinsic.

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1504–1505

I would be tempted to check if call N->getOperand() is what is expected.
But I believe once we are here these checks don't need to happen anymore. Correct?
I am just saying this because the operand's selection is according to a newly introduced parameter that I believe is asking if this is Intrinsic.
I believe before you had tablegen to help to check the types, but now you are using this function for other intrinsic.

3904

Just in case:
When I had to work with ld3 and make tests for it.
I needed to change the ld to accept these types:

2f16, 2b16
4f16, 4bf16  and
2f32

Should these types be here too?

david-arm added inline comments.Oct 20 2021, 12:33 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3904

I think that's a good question! It looks like the ACLE only defines versions of the intrinsics using the fully packed types, so perhaps we never expect to see unpacked types here?

paulwalker-arm added inline comments.Oct 20 2021, 2:51 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3904

That it correct. The majority of the SVE intrinsics only care about packed vectors. Although we've tried to declare them in such a way that they're largely element count agnostic, there is little support within the code generator for anything other than packed vectors.

bsmith added inline comments.Oct 20 2021, 5:27 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1504–1505

I don't think checking the result of N->getOperand() is necessary, I would think that what is essentially malformed IR would have been caught by this point.

Also I'm not sure that previously we had any help from tablegen here, the existing ld<n> intrinsics already used this function.

llvm/test/CodeGen/AArch64/sve-intrinsics-ldN-sret-reg+reg-addr-mode.ll
255

Nit: 2-indent for function bodies?

bsmith updated this revision to Diff 381194.Oct 21 2021, 3:32 AM
  • Fix formatting issues in tests and intrinsics definitions
peterwaller-arm accepted this revision.Oct 21 2021, 3:37 AM
This revision is now accepted and ready to land.Oct 21 2021, 3:37 AM
paulwalker-arm added inline comments.Oct 21 2021, 6:50 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3904

I see you're just matching the existing code so no action required, but for information we don't need this check. hasBF16 is only required to protect instruction usage and not the underlying datatypes. The reason being that much like in this case there's no need for any special BF16 instructions to implement this functionality.