[PATCH] D89105: [X86] Support Intel avxvnni

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: clang/lib/Headers/avxvnniintrin.h:32
+#include "commonvnniintrin.h"
+
+/* Intrinsics with _avx_ prefix are for compatibility with msvc. */
----------------
craig.topper wrote:
> pengfei wrote:
> > pengfei wrote:
> > > craig.topper wrote:
> > > > pengfei wrote:
> > > > > 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
> > > > > ```
> > > > gcc appears to have just put the common intrinsics in avxvnniintrin.h. Why can't we do that?
> > > 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.
> > 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.
> 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.
Right. Thank @craig.topper 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89105/new/

https://reviews.llvm.org/D89105