This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add AVX-VNNI-INT8 instructions.
ClosedPublic

Authored by FreddyYe on Oct 13 2022, 8:20 PM.

Diff Detail

Event Timeline

FreddyYe created this revision.Oct 13 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2022, 8:20 PM
FreddyYe requested review of this revision.Oct 13 2022, 8:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2022, 8:20 PM
craig.topper added inline comments.
clang/lib/Basic/Targets/X86.cpp
793

Why is this define needed?

996

This appears to have been in alpabetical order before.

clang/lib/Headers/immintrin.h
262

Why is this define needed?

RKSimon added inline comments.Oct 14 2022, 1:27 AM
clang/test/CodeGen/avxvnniint8-builtins.c
2

32-bit test coverage?

I'm out of machines next two days. Sorry for late address in advance... I'll update next Monday. Thanks for review!

FreddyYe updated this revision to Diff 468144.Oct 17 2022, 3:02 AM

Address comments. THX for review!

FreddyYe marked 4 inline comments as done.Oct 17 2022, 3:03 AM

THX for review!

RKSimon added inline comments.Oct 17 2022, 3:35 AM
clang/docs/ReleaseNotes.rst
637

Please add a bullet list of the added intrinsics

clang/lib/Headers/avxvnniint8intrin.h
33

Please add doxygen descriptions for each intrinsic

llvm/test/MC/Disassembler/X86/avx-vnni_int8-att.txt
1 ↗(On Diff #468144)

I think we'd be better off merging the att/intel test files and using --check-prefix

FreddyYe marked 2 inline comments as done.Oct 17 2022, 7:52 PM
FreddyYe added inline comments.
llvm/test/MC/Disassembler/X86/avx-vnni_int8-att.txt
1 ↗(On Diff #468144)

I see many old tests split them into two files. What about remove 32bit att test and 64 bit intel test, it can also help reduce code base?

FreddyYe updated this revision to Diff 468398.Oct 17 2022, 8:00 PM

Address comments.

craig.topper added inline comments.Oct 17 2022, 8:13 PM
llvm/lib/Target/X86/X86InstrSSE.td
8149

There are two spaces before T8XD. Same the next few instructions

llvm/test/CodeGen/X86/avxvnniint8-intrinsics.ll
2

This is only testing half the intrinsics. I think you test the non-S for 128 and the S for 256.

7

Are there tests for commuting?

Can you fix the MC + disasm test file names - drop att/intel and ensure you test both syntaxes for 32 and 64 bits.

Ideally the 32/64 bit names should be close to each other in a file list (e.g. avx-vnni-int8-32.s + avx-vnni-int8-64.s ?)

FreddyYe marked 2 inline comments as done.Oct 18 2022, 8:18 PM

Can you fix the MC + disasm test file names - drop att/intel and ensure you test both syntaxes for 32 and 64 bits.

Ideally the 32/64 bit names should be close to each other in a file list (e.g. avx-vnni-int8-32.s + avx-vnni-int8-64.s ?)

I get your point of "close to each other" and updated. And I merged the Disasm tests, while I didn't merge the MC tests because it is not so convenient to do. See latest updated.

Do we need to rename old tests to follow this rule? Old tests: https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/X86 and https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/Disassembler/X86

FreddyYe added inline comments.Oct 18 2022, 8:20 PM
llvm/test/CodeGen/X86/avxvnniint8-intrinsics.ll
7

Hi Craig,
Can you show an example of commutable for source operands but none of then are destination? I cannot figure out a good way to add such test.

craig.topper added inline comments.Oct 18 2022, 8:27 PM
llvm/test/CodeGen/X86/avxvnniint8-intrinsics.ll
7

See stack_fold_vpdpwssd_commuted in stack-folding-int-avxvnni.ll

FreddyYe updated this revision to Diff 468772.Oct 18 2022, 8:28 PM

Address comments. THX for review.

FreddyYe updated this revision to Diff 468783.Oct 18 2022, 10:17 PM
FreddyYe marked 2 inline comments as done.

Address comment. Add commute tests.

llvm/test/CodeGen/X86/avxvnniint8-intrinsics.ll
7

Got it. Thanks!

Matt added a subscriber: Matt.Oct 19 2022, 4:59 PM
pengfei added inline comments.Oct 19 2022, 6:53 PM
clang/lib/Headers/CMakeLists.txt
149

Move it before avxvnniintrin.h for the order?

llvm/lib/Support/X86TargetParser.cpp
585

This is redundant, I have fixed ImpliedFeaturesAVX512FP16 in trunk code.

I get your point of "close to each other" and updated. And I merged the Disasm tests, while I didn't merge the MC tests because it is not so convenient to do. See latest updated.

Do we need to rename old tests to follow this rule? Old tests: https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/X86 and https://github.com/llvm/llvm-project/tree/main/llvm/test/MC/Disassembler/X86

Its not a priority, but if you are ever bored and want to do some cleaning then it help!

FreddyYe marked 2 inline comments as done.Oct 20 2022, 8:50 PM

Its not a priority, but if you are ever bored and want to do some cleaning then it help!

I see. Then we are on the same side. I'll clean if I had time after landing these patches.

FreddyYe updated this revision to Diff 469463.Oct 20 2022, 8:52 PM

Address comments and rebase.

skan added inline comments.Oct 23 2022, 11:37 PM
llvm/include/llvm/Support/X86TargetParser.def
206

Move it after AVXVNNI to keep the dictionary order?

llvm/lib/Target/X86/X86InstrSSE.td
8127–8131

Could you unify the name converntion? e.g Captialize the first character for all parameters for this multiclass.

llvm/test/CodeGen/X86/stack-folding-int-avxvnniint8.ll
5–6

Could we remove these two lines?

RKSimon added inline comments.Oct 26 2022, 2:55 AM
clang/include/clang/Basic/BuiltinsX86.def
2119

Add ' AVX-VNNI-INT8' title comment like we have for other intrinsic sets

2132

Move these earlier with the other vnni builtins

llvm/lib/Target/X86/X86ISelLowering.h
592

Do we actually need these? Are you intending to add DAG combines for these?

FreddyYe updated this revision to Diff 471023.Oct 26 2022, 10:28 PM
FreddyYe marked 6 inline comments as done.

Address comments. THX for review!

FreddyYe added inline comments.Oct 26 2022, 10:34 PM
llvm/include/llvm/Support/X86TargetParser.def
206

Better refine at another patch since it's not ordered already

llvm/lib/Target/X86/X86ISelLowering.h
592

Yes. A continued patch will support DAG combine like old VNNI instructions. https://reviews.llvm.org/D116039

FreddyYe updated this revision to Diff 471029.Oct 26 2022, 10:58 PM

minor fix.

pengfei accepted this revision.Oct 27 2022, 12:54 AM

LGTM.

This revision is now accepted and ready to land.Oct 27 2022, 12:54 AM
skan accepted this revision.Oct 27 2022, 1:46 AM

LGTM

FreddyYe updated this revision to Diff 471361.Oct 27 2022, 6:52 PM

Rebase. THX for all of the review!

FreddyYe updated this revision to Diff 471376.Oct 27 2022, 7:33 PM

minor fix.

This revision was landed with ongoing or failed builds.Oct 27 2022, 7:40 PM
This revision was automatically updated to reflect the committed changes.