Page MenuHomePhabricator

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

Authored by paulwalker-arm on Thu, May 26, 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

Event Timeline

paulwalker-arm created this revision.Thu, May 26, 9:49 AM
paulwalker-arm requested review of this revision.Thu, May 26, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 26, 9:49 AM
peterwaller-arm accepted this revision.Mon, May 30, 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
11373

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

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

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.Mon, May 30, 5:54 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11357–11361

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.Mon, May 30, 5:58 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11357–11361

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.Mon, May 30, 6:04 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11357–11361

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.Mon, May 30, 6:10 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11357–11361

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.Mon, May 30, 6:24 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11357–11361

Sure, will do.

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

Fixed typos and improved comments.

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

LGTM! Thanks @paulwalker-arm. :)

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