This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add SHA512 instructions.
ClosedPublic

Authored by FreddyYe on Jul 12 2023, 7:16 PM.

Diff Detail

Event Timeline

FreddyYe created this revision.Jul 12 2023, 7:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 7:16 PM
FreddyYe requested review of this revision.Jul 12 2023, 7:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 12 2023, 7:16 PM
pengfei added inline comments.Jul 13 2023, 5:42 AM
clang/lib/Headers/CMakeLists.txt
208

alphabetical order

llvm/include/llvm/IR/IntrinsicsX86.td
5112

DefaultAttrsIntrinsic

5114

ditto.

5116

ditto.

llvm/test/CodeGen/X86/sha512-intrinsics.ll
3

X64,CHECK

3

No need O0

4

X86,CHECK

4

ditto.

llvm/test/MC/Disassembler/X86/sha512-64.txt
2–3

This can be merged with sha512-32.txt

llvm/test/MC/X86/sha512-att-64.s
1 ↗(On Diff #539826)

ditto.

llvm/test/MC/X86/sha512-intel-64.s
1 ↗(On Diff #539826)

ditto.

craig.topper added inline comments.Jul 13 2023, 11:12 AM
llvm/lib/Target/X86/X86.td
243

AVX2 like other integer features?

llvm/lib/Target/X86/X86InstrSSE.td
8304

This paren should be indented 1 more space so that it's not at the same column as the one above it.

8310

ditto

8316

ditto

llvm/lib/TargetParser/X86TargetParser.cpp
214

Unnecessary line break

658

Should this be AVX2 like all the other integer features?

pengfei added inline comments.Jul 13 2023, 7:01 PM
llvm/lib/Target/X86/X86.td
243

ISE says it only requires AVX

FreddyYe updated this revision to Diff 540885.Jul 17 2023, 12:34 AM
FreddyYe marked 15 inline comments as done.

Address comments.

skan accepted this revision.Jul 17 2023, 12:38 AM

LGTM

This revision is now accepted and ready to land.Jul 17 2023, 12:38 AM
FreddyYe updated this revision to Diff 540893.Jul 17 2023, 1:13 AM

Added FIXME for schedule

FreddyYe updated this revision to Diff 540909.Jul 17 2023, 1:50 AM

Remove -check-prefix=CHECK

RKSimon added inline comments.Jul 17 2023, 3:18 AM
clang/lib/Headers/sha512intrin.h
22

doxygen descriptions?

clang/test/CodeGen/X86/sha512-builtins.c
6

why do you need stddef.h?

FreddyYe updated this revision to Diff 540951.Jul 17 2023, 4:33 AM

Remove #include <stddef.h>

FreddyYe retitled this revision from Add SHA512 instructions. to [X86] Add SHA512 instructions..Jul 17 2023, 4:41 AM
FreddyYe updated this revision to Diff 541392.Jul 18 2023, 1:27 AM
FreddyYe marked 2 inline comments as done.

Address comments.

RKSimon accepted this revision.Jul 18 2023, 2:35 AM

LGTM - but I'd prefer more complete 32-bit vs 64-bit test coverage (similar to the SM3/SM4 patches) if its possible.

FreddyYe updated this revision to Diff 541502.Jul 18 2023, 6:27 AM

Split assembly tests and cover more registers.

FreddyYe updated this revision to Diff 541503.Jul 18 2023, 6:29 AM

Remove -x86-asm-syntax=intel

FreddyYe updated this revision to Diff 541793.Jul 18 2023, 5:11 PM

Add missing doxygen

FreddyYe marked 3 inline comments as done.Jul 18 2023, 5:28 PM

I think we can discuss this issue first in https://reviews.llvm.org/D155662

This revision was landed with ongoing or failed builds.Jul 19 2023, 6:44 PM
This revision was automatically updated to reflect the committed changes.