[PATCH] D83111: [X86-64] Support Intel AMX Intrinsic

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Headers/amxintrin.h:28
+///
+/// \headerfile <amxintrin.h>
+///
----------------
xiangzhangllvm wrote:
> MaskRay wrote:
> > Should this be `\headerfile <x86intrin.h>` ?
> > 
> > Many other macros defined in other *intrin.h files use x86intrin.h
> 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.
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.


================
Comment at: clang/lib/Headers/amxintrin.h:33
+/// \param __config
+///    A pointer point to m512
+static __inline__ void __DEFAULT_FN_ATTRS
----------------
xiangzhangllvm wrote:
> MaskRay wrote:
> > `A pointer to a __m512.`
> Yes, It point to m512, but here the type is const void*, we need to sync with other compiler and SPEC.
The meaning of "m512" as an abbreviation isn't obvious. Say 64 byte memory location or 512-bit memory location


================
Comment at: clang/lib/Headers/amxintrin.h:130
+///    The destination tile to be zero. Max size is 1024 Bytes.
+#define _tile_zero(tile) \
+  __builtin_ia32_tilezero((tile))
----------------
xiangzhangllvm wrote:
> MaskRay wrote:
> > no need to wrap
> > 
> > this applies to many other macros below
> Sorry, do you mean needn't _tile_zero, just use __builtin_ia32_tilezero ?
> Its is API for users, other compiler will also follow.
I think he meant "line wrap". There is no need for the macro to be split across 2 lines


================
Comment at: llvm/test/CodeGen/X86/AMX/amx-int8-intrinsics.ll:17
+  ret void
+}
+
----------------
xiangzhangllvm wrote:
> RKSimon wrote:
> > its probably cleaner to split these and have one test for each intrinsic
> Hello, Simon, They are same feature intrinsic, So I put them together, It is common to check one more instructions in a function.
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.


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

https://reviews.llvm.org/D83111