This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add patterns for scalable vselect
ClosedPublic

Authored by cameron.mcinally on Dec 10 2019, 1:37 PM.

Details

Summary

This patch matches scalable vector selects to predicated move instructions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
efriedma accepted this revision.Dec 10 2019, 2:28 PM

LGTM

On a sort of related note, AArch64ISelLowering.cpp says that MVT::nxv1f32 and MVT::nxv1f64 are also legal? Do we plan to implement isel patterns for them?

This revision is now accepted and ready to land.Dec 10 2019, 2:28 PM
c-rhodes added a comment.EditedDec 11 2019, 5:36 AM

On a sort of related note, AArch64ISelLowering.cpp says that MVT::nxv1f32 and MVT::nxv1f64 are also legal? Do we plan to implement isel patterns for them?

nxv1f32 and nxv1f64 shouldn't be legal types, that was a mistake on my part
when implementing the initial calling convention for SVE. We avoid nxv1<eltty>
types as they can't be split if the element type is too big. We've also not had
to worry about these types from a vectorization point of view because the
vectorizer normally only generates VF=1 to indicate it wants to scalarize the
loop and in practice there is little value from vectorization when VF=vscale*1

MVT::nxv2f16 and MVT::nxv4f16 are legal types however so maybe it's worth
adding isel patterns for those in this patch?

I'll create a patch to remove nxv1f32 and nxv1f64 as legal types.

llvm/test/CodeGen/AArch64/sve-select.ll
2

Can this be removed? (I'm not sure if this test was generated?)

9–12

nit: can the CHECK lines be shifted down a couple of lines to the function body? It would be a little easier to read.

Thanks for adding these patterns @cameron.mcinally !

llvm/test/CodeGen/AArch64/sve-select.ll
21

nit: the check for the basic-block seems unnecessary.

cameron.mcinally marked 3 inline comments as done.Dec 11 2019, 2:26 PM
cameron.mcinally added inline comments.
llvm/test/CodeGen/AArch64/sve-select.ll
2

These were automatically generated...

9–12

And, yeah, I thought that was weird too. That's what update_llc_test_checks produced. I also see it in some other generated tests as well.

21

I can hand edit these if everyone wants that.

cameron.mcinally marked an inline comment as done.Dec 11 2019, 2:58 PM
cameron.mcinally added inline comments.
llvm/test/CodeGen/AArch64/sve-select.ll
21

llvm-dev says that this is the intended behavior. The fix is to put the function declaration on one line. That seems a little excessive for a case like this though.

I'll manually edit the CHECK lines and remove the automatic header note...

This revision was automatically updated to reflect the committed changes.