[PATCH] D141899: [IR][X86] Remove X86AMX type in LLVM IR instead of target extension

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added a comment.

>From some of the verifier checks and tests, it looks like `target("x86.amx")` would also require some new type properties, to express its unsuitability for alloca-and-friends, as well as non-intrinsic arguments.

The spelling change needs to be release noted, and I would like there to be some mention of the the type in documentation going forward, but it seems that X86 doesn't have a target-specific documentation page yet.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5338
+        if (TargetExtTy->getName() == "x86.AMX")
+          return true;
+      return false;
----------------
nikic wrote:
> Probably worthwhile to add an overload like `Type::isTargetExtTy(StringRef)`?
+1 to a method for this pattern.


================
Comment at: llvm/include/llvm-c/Core.h:168
   LLVMBFloatTypeKind,    /**< 16 bit brain floating point type */
-  LLVMX86_AMXTypeKind,   /**< X86 AMX */
   LLVMTargetExtTypeKind, /**< Target extension type */
----------------
Removing this enum value changes the ABI. I don't think we've removed a type before, but with analogy to the opcode enum above, we should probably explicitly enumerate the type kinds and leave a hole for where `LLVMX86_AMXTypeKind` used to be.


================
Comment at: llvm/include/llvm-c/Core.h:1555
- */
-LLVMTypeRef LLVMX86AMXTypeInContext(LLVMContextRef C);
-
----------------
Retaining the existing LLVM-C methods is possible without much maintenance overhead, so we should do so.


================
Comment at: llvm/lib/IR/Type.cpp:843
+      ArrayType::get(FixedVectorType::get(Type::getIntNTy(C, 32), 16), 16),
+      TargetExtType::HasZeroInit, TargetExtType::CanBeGlobal);
+  }
----------------
If I'm following the verifier rules for `x86_amx` correctly, these are not in fact true for a `target("x86.amx")` type.


================
Comment at: llvm/test/Verifier/x86_amx1.ll:2-3
-; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
-
- at GV = dso_local global [10 x x86_amx] zeroinitializer, align 16
-; CHECK: invalid array element type
----------------
It should be possible to retain these verifier tests even if `x86_amx` is moved to a target extension type, although the messages may need to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141899