This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Fixed custom lowering of ISD::INSERT_SUBVECTOR.
ClosedPublic

Authored by paulwalker-arm on May 26 2022, 9:49 AM.

Details

Summary

LowerINSERT_SUBVECTOR emits AArch64ISD::UUNPK## when lowering
scalable vector floating point INSERT_SUBVECTOR. However, these
nodes only make sense for integer types and thus isel patterns do
not exist for floating point, which leads to isel failures.

This patch ensures floating point operands are cast to integer
before the core lowering takes place.

Fixes: #55037

Diff Detail

Unit TestsFailed

Event Timeline

paulwalker-arm created this revision.May 26 2022, 9:49 AM
paulwalker-arm requested review of this revision.May 26 2022, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 9:49 AM
peterwaller-arm accepted this revision.May 30 2022, 4:35 AM

LGTM.

An observation: nounwind only appears to affect output for 5 tests, in case there is an appetite to remove it elsewhere: insert_v2i64_nxv2i64_idx2 / insert_v4i32_nxv4i32_idx4 / insert_v16i8_nxv16i8_idx16 / insert_nxv8f16_nxv2f16 / insert_nxv4bf16_v4bf16.

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

nit: s/concatinate/concatenate/ s/dependant/dependent/ or perhaps s/dependant/depending/.

This revision is now accepted and ready to land.May 30 2022, 4:35 AM
david-arm added inline comments.May 30 2022, 5:49 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

Are these names the wrong way around? I would have expected the wider VT to be called WideVT. We're inserting a narrower InVT subvector into a wider VT vector.

paulwalker-arm added inline comments.May 30 2022, 5:54 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

It's wider in the context of the element type since both types have the same total bit length. WideVT is the wider VT because it's created based on the element count. InVT (i.e. the subvector) has fewer elements than VT and thus its element type will need to be wider in order to match the total bit length of NarrowVT.

david-arm added inline comments.May 30 2022, 5:58 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

OK, fair enough. So the width or narrowness refers to the element types then? I just found it really confusing that's all, as intuitively I was expecting an insert subvector operation to insert a narrower VT into a wider one. I guess what you actually mean here is WideElementVT and NarrowElementVT. I was thinking of widening in the legalisation sense, i.e. widen a <vscale x 3 x f32> -> <vscale x 4 x f32>.

paulwalker-arm added inline comments.May 30 2022, 6:04 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

Although true for the original insert subvector operation the confusion here is that when lowering we're actually merging two vectors of equal length.

david-arm added inline comments.May 30 2022, 6:10 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

Yeah I see. In that case would it be possible to leave a simple comment before merging this patch just explaining that a bit of what you said earlier, i.e. that narrow and wide here refer to the element types?

paulwalker-arm added inline comments.May 30 2022, 6:24 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11480–11484

Sure, will do.

Allen added a subscriber: Allen.May 30 2022, 6:27 AM

Fixed typos and improved comments.

david-arm accepted this revision.May 31 2022, 12:57 AM

LGTM! Thanks @paulwalker-arm. :)

Matt added a subscriber: Matt.Jun 1 2022, 7:30 PM
This revision was landed with ongoing or failed builds.Jun 2 2022, 7:07 AM
This revision was automatically updated to reflect the committed changes.