This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Canonicalize ZERO_EXTEND to VSELECT
ClosedPublic

Authored by NicolaLancellotti on Oct 10 2022, 8:56 AM.

Details

Summary

This patch canonicalizes ZERO_EXTEND to VSELECT to allow zext to transform into a predicated instruction like add, sub or mul.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:56 AM
NicolaLancellotti requested review of this revision.Oct 10 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:56 AM
NicolaLancellotti edited the summary of this revision. (Show Details)Oct 10 2022, 9:01 AM
NicolaLancellotti added a reviewer: dmgreen.

This update removes an if statement that prevents the canonicalization

Given you want to canonicalise to vselect is it worth doing this during lowering and removing the existing zext isel pattern? that way we can be sure all instances use the same idiom? This looks like it would be equally useful for sign_extend's of predicates also?

Address comments

Given you want to canonicalise to vselect is it worth doing this during lowering and removing the existing zext isel pattern? that way we can be sure all instances use the same idiom? This looks like it would be equally useful for sign_extend's of predicates also?

Paul, thank you very much for the review.
I moved the code so that the canonicalisation is done during the lowering, and I removed the existing zext isel pattern.
You are right, it would be useful for sign_extend too, I'll create another patch later.

A couple of nits but it looks like the move to lowering has caused some side effects whereby we now return "Invalid cost" for predicate based llvm.experimental.vector.splice.nxv16i1. I think we're just missing some entries in AArch64TTIImpl::getCastInstrCost like

{ ISD::ZERO_EXTEND, MVT::nxv2i64, MVT::nxv2i1, 1 },
{ ISD::ZERO_EXTEND, MVT::nxv4i32, MVT::nxv4i1, 1 },
{ ISD::ZERO_EXTEND, MVT::nxv8i16, MVT::nxv8i1, 1 },
{ ISD::ZERO_EXTEND, MVT::nxv16i8, MVT::nxv16i1, 1 },
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
5545

Can use DL, VT here as above.

5765–5766

Please can you move these two lines into LowerZERO_EXTEND so all the zext lowering has the same entry point.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
515

Does (SVEDup0) work here as a shortcut?

Addressed comments

NicolaLancellotti marked 2 inline comments as done.Oct 13 2022, 8:19 AM
NicolaLancellotti added inline comments.
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
515

I think so, I've just replaced zext with vselect, because of the new lowering.

paulwalker-arm accepted this revision.Oct 13 2022, 5:27 PM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
5548–5549

This should all fit on one line now. You probably just need to rerun clang-format.

This revision is now accepted and ready to land.Oct 13 2022, 5:27 PM

Files have been reformatted.

This revision was landed with ongoing or failed builds.Oct 17 2022, 7:43 AM
This revision was automatically updated to reflect the committed changes.