This is an archive of the discontinued LLVM Phabricator instance.

[X86-64] Support Intel AMX Intrinsic
ClosedPublic

Authored by xiangzhangllvm on Jul 2 2020, 8:15 PM.

Details

Summary

INTEL ADVANCED MATRIX EXTENSIONS (AMX).
AMX is a new programming paradigm, it has a set of 2-dimensional registers
(TILES) representing sub-arrays from a larger 2-dimensional memory image and
operate on TILES.

These intrinsics use direct TMM register number as its params.

Spec can be found in Chapter 3 here https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Jul 2 2020, 8:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 8:15 PM
craig.topper added inline comments.Jul 3 2020, 4:40 PM
clang/lib/Headers/amxintrin.h
23

The format isn't described below. I don't think it needs to be. But we shouldn't say it is described.

42

same

clang/lib/Sema/SemaChecking.cpp
3613

Why are Low/High arguments? Aren't they always 0 and 7.

xiangzhangllvm marked an inline comment as done.Jul 3 2020, 10:47 PM
xiangzhangllvm added inline comments.
clang/lib/Sema/SemaChecking.cpp
3613

Yes, they are 0 and 7, keep the Low and High will make it easy to read.

craig.topper added inline comments.Jul 3 2020, 11:14 PM
clang/lib/Sema/SemaChecking.cpp
3613

Its sort of confusing to use a function arguments where the default value is always used. Would it be better to have clearly named static const global variables?

3630

Would probably make sense to have a named constant for this 8 here and the other one below.

RKSimon added inline comments.Jul 4 2020, 12:15 AM
llvm/test/CodeGen/X86/AMX/amx-int8-intrinsics.ll
18

its probably cleaner to split these and have one test for each intrinsic

llvm/test/CodeGen/X86/AMX/amx-tile-intrinsics.ll
24

its probably cleaner to split these and have one test for each intrinsic

MaskRay added inline comments.Jul 4 2020, 11:59 AM
clang/lib/Headers/amxintrin.h
2

Drop C++ is this file can be included as a C header.

29

Should this be \headerfile <x86intrin.h> ?

Many other macros defined in other *intrin.h files use x86intrin.h

34

A pointer to a __m512.

131

no need to wrap

this applies to many other macros below

clang/test/CodeGen/AMX/amx.c
3

indent a continuation line by 2 columns

-Wall does not appear to be used

clang/test/Driver/x86-target-features.c
236

--check-prefix= is much more common than other forms (e.g. -check-prefix )

please fix other occurrences as well

Also, don't add extra spaces like FileCheck --check-prefix

llvm/lib/Target/X86/X86ISelLowering.cpp
33048

assert(

delete trailing period.

33360

The two new lines don't appear to improve readability

xiangzhangllvm marked 5 inline comments as done.Jul 4 2020, 8:36 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/amxintrin.h
29

The relation of them is "x86intrin.h" include "immintrin.h" include amxintrin.h.
In fact, some info of these comment generated it from our other requirement files.
the amxintrin.h in it tell us should put this intrinsics in amxintrin.h.

34

Yes, It point to m512, but here the type is const void*, we need to sync with other compiler and SPEC.

clang/test/Driver/x86-target-features.c
236

Oh, seem right, but do you think a AMX patch is better to change the codes which have no relation with it ?
And, sorry,not much understand "FileCheck --check-prefix", does you mean 2 spaces between "FileCheck" and "--check-prefix" ?

llvm/lib/Target/X86/X86ISelLowering.cpp
33360

The 2 blank lines ?

llvm/test/CodeGen/X86/AMX/amx-int8-intrinsics.ll
18

Hello, Simon, They are same feature intrinsic, So I put them together, It is common to check one more instructions in a function.

xiangzhangllvm added inline comments.Jul 4 2020, 8:46 PM
clang/lib/Headers/amxintrin.h
131

Sorry, do you mean needn't _tile_zero, just use __builtin_ia32_tilezero ?
Its is API for users, other compiler will also follow.

craig.topper added inline comments.Jul 4 2020, 9:14 PM
clang/lib/Headers/amxintrin.h
29

The doxygen is for the user of the compiler. They can't include amxintrin.h without getting a #error. So the header listed here needs to be something that the user can do.

34

The meaning of "m512" as an abbreviation isn't obvious. Say 64 byte memory location or 512-bit memory location

131

I think he meant "line wrap". There is no need for the macro to be split across 2 lines

llvm/test/CodeGen/X86/AMX/amx-int8-intrinsics.ll
18

Its not that common. Most of the time is only for testing the unmasked and masked version in avx512. And I split a lot of those a few months ago.

MaskRay added inline comments.Jul 4 2020, 9:21 PM
clang/test/Driver/x86-target-features.c
236

You wrote FileCheck -check-prefix below.

Please use FileCheck --check-prefix=

xiangzhangllvm marked 13 inline comments as done.Jul 4 2020, 9:33 PM

Fix some missed change last time.
1 doxygen comments: amxintrin.h --> x86intrin.h

refine ldtilecfg and sttilecfg comment.

2 replace tile reg num 8 with TileRegHigh+1

craig.topper accepted this revision.Jul 6 2020, 6:35 PM

LGTM with all instances of "pointer point" replace with just "pointer"

clang/lib/Headers/amxintrin.h
34

Drop 'point' here. I think its understood that a pointer points. And it would need to be "pointing" if it stays

This revision is now accepted and ready to land.Jul 6 2020, 6:35 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 7:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

LGTM with all instances of "pointer point" replace with just "pointer"

Done it in commit. Thank you!