This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add S/UQXTRN tablegen patterns.
ClosedPublic

Authored by dmgreen on May 27 2021, 9:12 AM.

Details

Summary

I found these on my computer so cleaned them up and added extra tests/types. This adds simple patterns for signed and unsigned saturating extract narrow instructions. They combine a min/max/truncate into a single instruction, providing that the immediates on the min/max are correct for the saturation type. This is just handled in tablegen with some extra patterns.

v2i64->v2i32 is not handled here as the min/max nodes are not legal, making the lowering quite different.

Diff Detail

Event Timeline

dmgreen created this revision.May 27 2021, 9:12 AM
dmgreen requested review of this revision.May 27 2021, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 9:12 AM
RKSimon resigned from this revision.May 28 2021, 3:43 AM
dmgreen edited reviewers, added: sdesmalen, CarolineConcatto; removed: RKSimon.Jun 16 2021, 1:58 AM
sdesmalen added inline comments.Jun 16 2021, 4:24 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
4233

From the comment, I deduce that (v8i16 (AArch64NvCast (v2i64 (AArch64movi_edit (i32 85))))) <=> v8i16 splat(255). Is there a reason this is not using AArch64dup?
If so, can we use a more generic pattern fragment to see if something is a dup of <value>? That may cover more cases and would also make the pattern more readable.

Matt added a subscriber: Matt.Jun 19 2021, 7:24 AM
dmgreen added inline comments.Jun 21 2021, 12:45 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
4233

It will lower any constant that it can into a AArch64ISD::MOVIedit/AArch64ISD::MOVIshift/etc. These become the various flavours of MOVI instructions. They are no longer dup's at this stage, if they ever were.

I have created PatFrags names for the constants used here. hopefully that makes it easier to read.

dmgreen updated this revision to Diff 353289.Jun 21 2021, 12:45 AM
SjoerdMeijer accepted this revision.Jun 30 2021, 6:24 AM
SjoerdMeijer added a subscriber: SjoerdMeijer.

Looks reasonable to me.

This revision is now accepted and ready to land.Jun 30 2021, 6:24 AM
dmgreen added inline comments.Jun 30 2021, 6:28 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
4234

Sander pointed out that these should be 7F, not EF. Doh! I'll update those.

sdesmalen accepted this revision.Jun 30 2021, 7:11 AM

LGTM if you fix s/EF/7F/ before you land it!

llvm/lib/Target/AArch64/AArch64InstrInfo.td
4236

It is nearly impossible to understand how these PatLeafs work, but that's nothing to do with your patch, since BuildVectors are lowered earlier on. It would be nice if these would just have been dup() nodes instead, but I'll just rely on the tests that you have implemented these PatLeafs correctly. At least the names of the PatLeafs (when you fix s/EF/7F/) make it a bit more understandable, thanks for making that change!

dmgreen added inline comments.Jul 2 2021, 8:29 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
4236

I have in the past looking into using VECTOR_SPLAT as the canonical representation of a splat in MVE, as opposed to a BUILD_VECTOR. That would mean moving the VMOVimm nodes into tablegen. It would involve a lot a changes, but feels like a sensible direction to me in the long run.

This revision was landed with ongoing or failed builds.Jul 2 2021, 11:57 PM
This revision was automatically updated to reflect the committed changes.