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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm marked 5 inline comments as done.
xiangzhangllvm added inline comments.


================
Comment at: clang/lib/Headers/amxintrin.h:28
+///
+/// \headerfile <amxintrin.h>
+///
----------------
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.


================
Comment at: clang/lib/Headers/amxintrin.h:33
+/// \param __config
+///    A pointer point to m512
+static __inline__ void __DEFAULT_FN_ATTRS
----------------
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.


================
Comment at: clang/test/Driver/x86-target-features.c:236
+
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mamx-tile %s -### -o %t.o 2>&1 | FileCheck -check-prefix=AMX-TILE %s
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-amx-tile %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-AMX-TILE %s
----------------
MaskRay wrote:
> `--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`
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" ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:33324
+    unsigned Imm = MI.getOperand(0).getImm();
+
+    BuildMI(*BB, MI, DL, TII->get(X86::TILEZERO), TMMImmToTMMReg(Imm));
----------------
MaskRay wrote:
> The two new lines don't appear to improve readability
The 2 blank lines ?


================
Comment at: llvm/test/CodeGen/X86/AMX/amx-int8-intrinsics.ll:17
+  ret void
+}
+
----------------
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.


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

https://reviews.llvm.org/D83111