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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/Headers/amxintrin.h:1
+/*===--------------- amxintrin.h - AMX intrinsics -*- C++ -*-----------------===
+ *
----------------
Drop C++ is this file can be included as a C header.


================
Comment at: clang/lib/Headers/amxintrin.h:28
+///
+/// \headerfile <amxintrin.h>
+///
----------------
Should this be `\headerfile <x86intrin.h>` ?

Many other macros defined in other *intrin.h files use x86intrin.h


================
Comment at: clang/lib/Headers/amxintrin.h:33
+/// \param __config
+///    A pointer point to m512
+static __inline__ void __DEFAULT_FN_ATTRS
----------------
`A pointer to a __m512.`


================
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))
----------------
no need to wrap

this applies to many other macros below


================
Comment at: clang/test/CodeGen/AMX/amx.c:2
+// RUN: %clang_cc1 %s -ffreestanding -triple=x86_64-unknown-unknown  -target-feature +amx-int8 \
+// RUN: -target-feature +amx-bf16 -emit-llvm -o - -Wall -Werror -pedantic | FileCheck %s --check-prefixes=CHECK
+
----------------
indent a continuation line by 2 columns

-Wall does not appear to be used


================
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
----------------
`--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`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:33012
+  auto TMMImmToTMMReg = [](unsigned Imm) {
+    assert (Imm < 8 && "Illegal tmm index.");
+    return X86::TMM0 + Imm;
----------------
`assert(` 

delete trailing period.


================
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));
----------------
The two new lines don't appear to improve readability


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

https://reviews.llvm.org/D83111