Page MenuHomePhabricator

[AArch64][SVE] Add support for trunc to <vscale x N x i1>.
ClosedPublic

Authored by efriedma on Tue, Jul 14, 1:12 PM.

Details

Summary

This isn't a natively supported operation, so convert it to a mask+compare.

In addition to the operation itself, fix up some surrounding stuff to make the testcase work: we need concat_vectors on i1 vectors, we need legalization of i1 vector truncates, and we need to fix up all the relevant uses of getVectorNumElements().

Diff Detail

Event Timeline

efriedma created this revision.Tue, Jul 14, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 14, 1:12 PM
sdesmalen accepted this revision.Tue, Jul 14, 1:43 PM

LGTM, thanks!

llvm/test/CodeGen/AArch64/sve-trunc.ll
69

It seems our downstream compiler uses lsl with #(elementwidth-1) instead of and #0x1, but using and seems a bit more intuitive.

This revision is now accepted and ready to land.Tue, Jul 14, 1:43 PM
paulwalker-arm added inline comments.Tue, Jul 14, 1:56 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19070

Could use getVectorMinNumElements() here.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2567

Since this is now an ElementCount this could be

assert(ElementCount / 2 && ....

which already asserts the division is lossless.

llvm/test/CodeGen/AArch64/sve-trunc.ll
77–144

Based on the assumption that these tests are really testing the CONCAT_VECTORS functionality, is it worth naming them as such (perhaps also move into sve-shuffles.ll or something)?

efriedma marked 2 inline comments as done.Tue, Jul 14, 3:35 PM
efriedma added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
2567

We do that division a bit later in the code.

I could drop the assertion, I guess.

llvm/test/CodeGen/AArch64/sve-trunc.ll
77–144

Well, it's testing the truncate itself, and the legalization of scalable truncates, and yes, also the concat_vectors functionality. I can rename them, sure; maybe trunc_i64toi1_split_op_and_concatenate_result. I think the location is fine, since the way it's expressed in IR is just a plain truncate.

paulwalker-arm added inline comments.Tue, Jul 14, 3:54 PM
llvm/test/CodeGen/AArch64/sve-trunc.ll
77–144

I guess I was just wondering if you would have still written these tests if there was a route to test CONCAT_VECTORS in isolation (of which I understand there currently isn't). If you're happy with the location then I'm happy with their current names.

efriedma marked an inline comment as done.Tue, Jul 14, 4:05 PM
efriedma added inline comments.
llvm/test/CodeGen/AArch64/sve-trunc.ll
77–144

We need these tests to test the legalization aspect, which can't be tested any other way.

paulwalker-arm accepted this revision.Wed, Jul 15, 4:25 AM

LGTM as long as we can replace the two instances of ElementCount.Min as suggested.

llvm/test/CodeGen/AArch64/sve-trunc.ll
77–144

Yep, understood.