[X86] Support Intel avxvnni

[LiuChen3]

Summary

This patch mainly made the following changes:

1. Support AVX-VNNI instructions;
2. Support more complex FeatureList. By default, ',' is prior than '|'. And parentheses is supported;
3. Introduce ExplicitVEXPrefix flag so that vpdpbusd/vpdpbusds/vpdpbusds/vpdpbusds instructions only use vex-encoding when user explicity add {vex} prefix.

[RKSimon]

Please can you add an entry to the 12.0 release notes?

clang/include/clang/Basic/BuiltinsNVPTX.def

46 ↗

(On Diff #297148)

Pull this out? This and the CodeGenFunction.* changes seem to be worth a separate patch

clang/lib/Headers/avxvnniintrin.h

32

Is having a commonvnniintrin.h header the approach gcc/icc are taking?

40

Please can you add doxygen descriptions to each intrinsic.

[LiuChen3]

Address comments.

[LiuChen3]

clang/include/clang/Basic/BuiltinsNVPTX.def

46 ↗

(On Diff #297148)

Thanks for your review.
I will separate this part. I want to keep this part here for now until another patch is merged in.

clang/lib/Headers/avxvnniintrin.h

32

GCC doesn't have commonvnniintrin.h headr, they only have avxvnniintrin.h. For ICC, I will check with this and give you answer later.

[LiuChen3]

clang/lib/Headers/avxvnniintrin.h

32

GCC and ICC doesn't break intrinsics into multiple headers.

[LiuChen3]

Rebase.

[RKSimon]

clang/lib/Headers/avxvnniintrin.h

32

Is there anyway that we can avoid this as well then? I get worried when header layouts start to diverge...

[craig.topper]

We also need to add avxvnni versions of the patterns from this commit along with appropriate testing. Maybe just split the 128/256 bit out of avx512vnni.ll and into a separate test with multiple RUN lines. That should all be done in a separate patch.

{code}
commit cc4b0596b1b0e58672e4151396c7b804eccaf273
Author: Craig Topper <[e-mail]>
Date: Sat Aug 24 23:14:57 2019 +0000

[X86] Add isel patterns to match vpdpwssd avx512vnni instruction from add+pmaddwd nodes.

{code}

[pengfei]

clang/lib/Headers/avxvnniintrin.h

32

Hi Simon, why we need to keep the same layout with GCC and ICC?
We always create new header files for new instructions. We don't allow user include these headers directly. So I think it doesn't matter if GCC and ICC create the same headers or not.

[craig.topper]

clang/lib/Headers/avxvnniintrin.h

32

As far as I know our header layout is largely the same as gcc for existing intrinsics. Can we just keep them the same?

clang/lib/Headers/commonvnniintrin.h

48 ↗

(On Diff #298920)

Arguments to macros shouldn't start with __

49 ↗

(On Diff #298920)

Because this is a macro this needs to have parentheses around S, A, and B.

[pengfei]

clang/lib/Headers/avxvnniintrin.h

33

Maybe we can move commonvnniintrin.h to immintrin.h. Then we don't have a different layout.

#if !(defined(_MSC_VER) || defined(__SCE__)) || __has_feature(modules) ||      \
     defined(__AVX512VNNI__) || (defined(__AVX512VL__) && defined(__AVX512VNNI__))
#include <commonvnniintrin.h>
#endif

[craig.topper]

clang/lib/Headers/avxvnniintrin.h

33

gcc appears to have just put the common intrinsics in avxvnniintrin.h. Why can't we do that?

[pengfei]

clang/lib/Headers/avxvnniintrin.h

33

I asked GCC guys. They don't use macro like defined(__AVXVNNI__). If we put it in avxvnniintrin.h, we need handle macro AVX512VNNI in avxvnniintrin.h. It's bit confused.

@LiuChen3 I meant __AVXVNNI__ for the first __AVX512VNNI__ in last comment.

[pengfei]

clang/lib/Headers/avxvnniintrin.h

33

Or maybe we can put the common intrinsics in avx512vlvnniintrin.h and not to handle macro __AVXVNNI__ int it.
I think it's reasonable. For Linux, we always include all headers. For Windows, we can only allow user use the _avx_ intrinscis for AVXVNNI, which is compatible with MSVC.

[craig.topper]

clang/lib/Headers/avxvnniintrin.h

33

Minor clarification. It's not Linux vs Windows. Its -ms-compatibility that controls it. You can use clang on Windows without enabling MSVC compatibility. In which case MSC_VER won't be defined.

[pengfei]

clang/lib/Headers/avxvnniintrin.h

33

Right. Thank @craig.topper

[RKSimon]

clang/test/CodeGen/avxvnni-builtins.c

1 ↗

(On Diff #298920)

Please can you move this test file under the X86 subdirectory?

[LiuChen3]

1. move the commonvnniintrin.h to the avx512vlvnniintrin.h.
2. move the testcase avxvnni-builtins.c to X86 subdirectory.

[craig.topper]

llvm/lib/Support/X86TargetParser.cpp

211

Add here too.

llvm/lib/Target/X86/X86.td

775

Please add this to Alderlake which was added to X86.td by @bkramer yesterday

[craig.topper]

Also need clang/test/Preprocessor/predefined-arch-macros.c updates for sapphirerapids and alderlake to check that AVXVNNI is defined.

[LiuChen3]

Adding avxvnni to Alder Lake.

[pengfei]

clang/test/Preprocessor/predefined-arch-macros.c

1637

You need to add to sapphirerapids too.

1708

You need to add to sapphirerapids too.

llvm/docs/ReleaseNotes.rst

124

You also need to add it to Clang release notes.

[LiuChen3]

Address comments.

[pengfei]

clang/lib/Headers/avxvnniintrin.h

31

I think we may still need add some information for intrinsics that can be used for AVXVNNI but not declared here. A proposal might be:

/* Below intrinsics defined in avx512vlvnniintrin.h can be used for */
/* AVXVNNI except used for compatibility with msvc.                 */
// #define _mm256_dpbusd_epi32(S, A, B)
// #define _mm256_dpbusds_epi32(S, A, B)
// #define _mm256_dpwssd_epi32(S, A, B)
...

[pengfei]

clang/lib/Headers/avxvnniintrin.h

31

I had a look at the Doxygen grammar. Maybe we can use like below:

/* Below intrinsics defined in avx512vlvnniintrin.h can be used for */
/* AVXVNNI except used for compatibility with msvc.                 */
/// \fn __m256i _mm256_dpbusd_epi32(__m256i __S, __m256i __A, __m256i __B)
/// \fn __m256i _mm256_dpbusds_epi32(__m256i __S, __m256i __A, __m256i __B)
/// \fn __m256i _mm256_dpwssd_epi32(__m256i __S, __m256i __A, __m256i __B)
...

[pengfei]

clang/lib/Headers/avxvnniintrin.h

32

Missing one line comment?

clang/lib/Headers/immintrin.h

144

Change back the spaces.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3851

Missing check for VEX2?

[LiuChen3]

Address comments

[pengfei]

LGTM with little nit:

llvm/test/CodeGen/X86/avx-vnni/avx_vnni-intrinsics.ll

1 ↗

(On Diff #301814)

Do we need new directory for the tests?

2 ↗

(On Diff #301816)

I don't see difference between AVX-X86 and AVX-X64, maybe you can use AVX for both, or AVX,AVX-X86 and AVX,AVX-X64 if they do have some differences. And remove CHECK since it never been used.

[LiuChen3]

1. Move the testcase from avx-vnni/ to test/CodeGen/X86/.
2. Refine the Run line in avx_vnni-intrinsics.ll

[pengfei]

LGTM. Thanks. Better to wait one day or two to see if others object.

[LiuChen3]

In D89105#2363927, @pengfei wrote:

LGTM. Thanks. Better to wait one day or two to see if others object.

Sure. Thanks for your review.

[RKSimon]

LGTM cheers

[craig.topper]

LGTM to me with a couple minors that can be fixed on commit.

clang/test/Preprocessor/predefined-arch-macros.c

1827–1828

doesn't this NOT need to be above AVXNNI to be effective since that's where AVX512 would go alphabetically?

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp

351

Put parentheses around the & parts. Similar to lock and notrack above. I don't want to think about operator precedence.

[LiuChen3]

Thanks for all of your review!