This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Adding SHA3 Intrinsics support
ClosedPublic

Authored by rsanthir.quic on Feb 9 2021, 4:23 PM.

Details

Summary
This patch adds the following SHA3 Intrinsics:
    vsha512hq_u64,
    vsha512h2q_u64,
    vsha512su0q_u64,
    vsha512su1q_u64
    veor3q_u8
    veor3q_u16
    veor3q_u32
    veor3q_u64
    veor3q_s8
    veor3q_s16
    veor3q_s32
    veor3q_s64
    vrax1q_u64
    vxarq_u64
    vbcaxq_u8
    vbcaxq_u16
    vbcaxq_u32
    vbcaxq_u64
    vbcaxq_s8
    vbcaxq_s16
    vbcaxq_s32
    vbcaxq_s64

Note need to include +sha3 and +crypto when building from the front-end

Diff Detail

Event Timeline

rsanthir.quic created this revision.Feb 9 2021, 4:23 PM
rsanthir.quic requested review of this revision.Feb 9 2021, 4:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 9 2021, 4:23 PM

This is the second of three patches to address the following:
https://bugs.llvm.org/show_bug.cgi?id=47828

apazos added inline comments.Feb 10 2021, 9:14 AM
clang/utils/TableGen/NeonEmitter.cpp
2118–2122

Consider alphabetizing the check. move isVXAR check after isVCT_N

alphabetized check in NeonEmitter

This looks like a straightforward implementation. The only caveat is that the XAR immediate does not represent a lane, and hence the need for a custom immediate range check. Looks sensible to me.
@labrinea and others at ARM, do have any other comment before this is merged?

One nit for now, I'll take a proper look tomorrow. Thanks for your work on these!

clang/test/CodeGen/aarch64-neon-sha3.c
27

Nit: remove empty lines in this and the following

rsanthir.quic marked an inline comment as done.Feb 17 2021, 10:04 AM

Removed extra whitespace

The approach for xar looks fine to me, matches how we handled vcvt_n_* (https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics?search=vcvt_n_).

clang/include/clang/Basic/arm_neon.td
1139

Put the second set in c-s-i-l order like the first.

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

This is unused.

llvm/test/CodeGen/AArch64/neon-sha3.ll
105

I'm not sure you need to declare variants you aren't using but in any case you should test the missing ones e.g. crypto.eor3u.v8i16.
Maybe there's an argument that it isn't very useful to test them all like this but the other files follow this pattern so might as well.

rsanthir.quic marked 3 inline comments as done.

Minor corrections and removed unused code, also added complete testing

rsanthir.quic added inline comments.Feb 18 2021, 11:32 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
928

a good catch thank you!

This revision is now accepted and ready to land.Feb 19 2021, 1:56 AM

Thank you for reviewing this @DavidSpickett ! If you get a chance could you commit this for me? I do not have commit access yet.

This revision was automatically updated to reflect the committed changes.
DavidSpickett added inline comments.Feb 22 2021, 4:10 AM
clang/test/CodeGen/aarch64-neon-range-checks.c
31

I added a "}" here. Please run ninja check-clang and/or check-llvm before you update patches.