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
-
-@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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to