This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add SM3 instructions.
ClosedPublic

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

Diff Detail

Event Timeline

FreddyYe created this revision.Jul 12 2023, 7:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 7:20 PM
FreddyYe requested review of this revision.Jul 12 2023, 7:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 12 2023, 7:20 PM
pengfei added inline comments.Jul 13 2023, 5:53 AM
llvm/test/MC/Disassembler/X86/sm3-32.txt
2

Remove blank line

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

Remove

5

We can merge 64-bit tests into 32-bit ones. The same below.

skan added inline comments.Jul 13 2023, 6:52 AM
llvm/lib/Target/X86/X86InstrSSE.td
8331

Is the schedule appropriate?

llvm/test/MC/Disassembler/X86/sm3-32.txt
4

Drop -x86-asm-syntax=intel

FreddyYe updated this revision to Diff 540892.Jul 17 2023, 1:12 AM
FreddyYe marked 5 inline comments as done.

Address comments.

FreddyYe added inline comments.Jul 17 2023, 1:13 AM
llvm/lib/Target/X86/X86InstrSSE.td
8331

I referred to SHA1MSG1's, any good ideas? I'll add a FIXME first here.

llvm/test/MC/Disassembler/X86/sm3-64.txt
5

Due to these instructions support rm form, hard to merge here?

RKSimon added inline comments.Jul 17 2023, 1:58 AM
clang/lib/Headers/sm3intrin.h
22

Doxygen descriptions?

llvm/test/MC/Disassembler/X86/sm3-64.txt
5

I'd prefer to see 32/64 test coverage kept separate, but it'd be more useful for 64 to use xmm8-xmm15 and 64-specific gpr registers etc. to increase coverage.

FreddyYe updated this revision to Diff 540949.Jul 17 2023, 4:30 AM

Remove #include <stddef.h>

FreddyYe retitled this revision from Add SM3 instructions. to [X86] Add SM3 instructions..Jul 17 2023, 4:42 AM
FreddyYe updated this revision to Diff 541340.Jul 17 2023, 11:59 PM
FreddyYe marked 2 inline comments as done.

Address commments.

FreddyYe updated this revision to Diff 541343.Jul 18 2023, 12:07 AM

Refine doxygen

RKSimon added inline comments.Jul 18 2023, 2:24 AM
clang/lib/Headers/sm3intrin.h
32

VPDPBSSD ?

FreddyYe updated this revision to Diff 541495.Jul 18 2023, 6:15 AM
FreddyYe marked an inline comment as done.

Address comments.

@pengfei Are you happy with the intrinsics doxygen descriptions?

pengfei added inline comments.Jul 18 2023, 7:21 AM
clang/lib/Headers/sm3intrin.h
29

Add return type too.

86

ditto.

149

ditto.

231

Missing __ for variables.

pengfei added inline comments.Jul 18 2023, 7:24 AM
clang/lib/Headers/sm3intrin.h
70

DEST[MAX:128] := 0 the same to below.

FreddyYe updated this revision to Diff 541792.Jul 18 2023, 5:06 PM
FreddyYe marked 5 inline comments as done.

Address comments.

clang/lib/Headers/sm3intrin.h
231

MACROs prefer no __ for prefix and the operation defines A, B, C, ... So I used another naming convention in doxygen for parameters.

pengfei accepted this revision.Jul 18 2023, 7:15 PM

LGTM.

This revision is now accepted and ready to land.Jul 18 2023, 7:15 PM
pengfei added inline comments.Jul 18 2023, 7:17 PM
clang/lib/Headers/sm3intrin.h
162–165

The description should invert

FreddyYe updated this revision to Diff 541815.Jul 18 2023, 7:30 PM
FreddyYe marked an inline comment as done.

Address comment.

pengfei added inline comments.Jul 18 2023, 7:48 PM
clang/lib/Headers/sm3intrin.h
163

This is int

FreddyYe updated this revision to Diff 541824.Jul 18 2023, 8:08 PM
FreddyYe marked an inline comment as done.

Address comments.

FreddyYe updated this revision to Diff 542274.Jul 19 2023, 7:23 PM

rebase and fix lit fail

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