This is an archive of the discontinued LLVM Phabricator instance.

[X86] use macros to split GFNI intrinsics into different kinds
ClosedPublic

Authored by FreddyYe on Nov 4 2020, 9:48 PM.

Details

Summary

Tremont microarchitecture only has GFNI(SSE) version, not AVX and
AVX512 version. This patch is to avoid compiling fail on Windows when
using -march=tremont to invoke one of GFNI(SSE) intrinsic.

Diff Detail

Event Timeline

FreddyYe created this revision.Nov 4 2020, 9:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2020, 9:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
FreddyYe requested review of this revision.Nov 4 2020, 9:48 PM
craig.topper added inline comments.Nov 4 2020, 9:54 PM
clang/lib/Headers/gfniintrin.h
35

This needs to be more like

#if !(defined(_MSC_VER) || defined(SCE)) || __has_feature(modules) || \

(defined(__AVXVL__)  && define(__AVX512BW__))

We want all intrinsics to be defined for when not compiling for MSVC compatibility. Otherwise using attribute(target) doesn't work.

What compilation failure do you get without this? The feature checks are compile time optimization for MSVC because windows.h includes intrin.h.

The fails are all unknown type errors on Windows, since those typedefs are declared in other header files.
The error message goes like:

$ clang -march=tremont gfni.c
......
...\lib\clang\12.0.0\include\gfniintrin.h:129:37: error:
      unknown type name '__mmask16'
_mm_mask_gf2p8mul_epi8(__m128i __S, __mmask16 __U, __m128i __A, __m128i __B)
                                    ^
...\lib\clang\12.0.0\include\gfniintrin.h:137:25: error:
      unknown type name '__mmask16'
_mm_maskz_gf2p8mul_epi8(__mmask16 __U, __m128i __A, __m128i __B)
                        ^
......
...\lib\clang\12.0.0\include\gfniintrin.h:159:43: error:
      unknown type name '__m256i'
_mm256_maskz_gf2p8mul_epi8(__mmask32 __U, __m256i __A, __m256i __B)
                                          ^
...
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

The fails are all unknown type errors on Windows, since those typedefs are declared in other header files.
The error message goes like:

$ clang -march=tremont gfni.c
......
...\lib\clang\12.0.0\include\gfniintrin.h:129:37: error:
      unknown type name '__mmask16'
_mm_mask_gf2p8mul_epi8(__m128i __S, __mmask16 __U, __m128i __A, __m128i __B)
                                    ^
...\lib\clang\12.0.0\include\gfniintrin.h:137:25: error:
      unknown type name '__mmask16'
_mm_maskz_gf2p8mul_epi8(__mmask16 __U, __m128i __A, __m128i __B)
                        ^
......
...\lib\clang\12.0.0\include\gfniintrin.h:159:43: error:
      unknown type name '__m256i'
_mm256_maskz_gf2p8mul_epi8(__mmask32 __U, __m256i __A, __m256i __B)
                                          ^
...
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

Ok that makes sense. Maybe the easiest fix is just check that AVXINTRIN_H or AVX512BWINTRIN_H or __AVX512BWVLINTRIN_H is defined where there's a dependency.

Craig's method sounds good.
@FreddyYe , Why we check AVX512BW instead of AVX512F. I saw SDM says it depends on AVX512F.

Craig's method sounds good.

I think it may result in potential problem once we change the order of them in immintrin.h, it should be better to use the first method.

#if !(defined(_MSC_VER) || defined(SCE)) || __has_feature(modules) || \
    (defined(__AVX512VL__)  && define(__AVX512BW__))

Craig's method sounds good.

I think it may result in potential problem once we change the order of them in immintrin.h, it should be better to use the first method.

#if !(defined(_MSC_VER) || defined(SCE)) || __has_feature(modules) || \
    (defined(__AVX512VL__)  && define(__AVX512BW__))

But if the order changes then the code won't compile because _m512i or some other type won't be defined.

Craig's method sounds good.
@FreddyYe , Why we check AVX512BW instead of AVX512F. I saw SDM says it depends on AVX512F.

I was referring to the old implement and test case. Seems like a error introduced before.

Craig's method sounds good.

I think it may result in potential problem once we change the order of them in immintrin.h, it should be better to use the first method.

#if !(defined(_MSC_VER) || defined(SCE)) || __has_feature(modules) || \
    (defined(__AVX512VL__)  && define(__AVX512BW__))

But if the order changes then the code won't compile because _m512i or some other type won't be defined.

I was thinking we may lose the chance to know we change them in wrong order. But it should be OK since we have intrinsic tests. So I agree with you.

Craig's method sounds good.
@FreddyYe , Why we check AVX512BW instead of AVX512F. I saw SDM says it depends on AVX512F.

I was referring to the old implement and test case. Seems like a error introduced before.

mmask64 requires avx512bw. And 32xi16 and 64xi8 aren’t well supported without avx512bw. We used to split them always, but we only split specific instructions now.

We also need avx512bw for the selectb and selectw builtins used for masking.

Craig's method sounds good.
@FreddyYe , Why we check AVX512BW instead of AVX512F. I saw SDM says it depends on AVX512F.

I was referring to the old implement and test case. Seems like a error introduced before.

mmask64 requires avx512bw. And 32xi16 and 64xi8 aren’t well supported without avx512bw. We used to split them always, but we only split specific instructions now.

We also need avx512bw for the selectb and selectw builtins used for masking.

I see. Thank you!

FreddyYe updated this revision to Diff 303047.Nov 4 2020, 11:54 PM

Use header file macros instead.

pengfei added inline comments.Nov 5 2020, 12:02 AM
clang/lib/Headers/gfniintrin.h
36

__AVX512VLBWINTRIN_H is better.

57–64

missing comments

184

comments

FreddyYe updated this revision to Diff 303073.Nov 5 2020, 3:38 AM

Reorganize intrinsic orders to avoid using nested macros.

pengfei added inline comments.Nov 5 2020, 4:03 AM
clang/lib/Headers/gfniintrin.h
72–79

These 2 functions need to move to __AVX512BWINTRIN_H

154–155

These 2 functions need to move to __AVX512BWINTRIN_H

183–184

These 2 functions need to move to __AVX512BWINTRIN_H

FreddyYe marked 3 inline comments as done.Nov 5 2020, 4:31 AM
FreddyYe added inline comments.
clang/lib/Headers/gfniintrin.h
154–155

Thanks for review!

FreddyYe updated this revision to Diff 303087.Nov 5 2020, 4:56 AM
FreddyYe marked an inline comment as done.

Refine

This revision is now accepted and ready to land.Nov 5 2020, 10:01 AM
pengfei accepted this revision.Nov 5 2020, 9:01 PM

LGTM. Thanks.

This revision was landed with ongoing or failed builds.Nov 6 2020, 12:17 AM
This revision was automatically updated to reflect the committed changes.