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

Tue Jan 17 06:45:53 PST 2023

nikic added a comment.

>From a very cursory look, this looks pretty nice. It looks like we pretty much get rid of AMX-specific knowledge in the middle-end entirely, which is the point of target extension types.

Regarding bitcasts, it's worth mentioning that there was some recent discussion about allowing bitcasts on target types as one of the supported properties -- however, I think we wouldn't actually want to do that for AMX, because of the special properties it has. In particular, if we allow bitcasts, we should also allow bitcast optimizations, and those are generally illegal for AMX (as seen with the various checks this patch removes).

Something this is missing is auto-upgrade support in the bitcode reader.

Nitpick: I wonder whether the type should be called `target("x86.amx")` -- it seems like lowercase names are more idiomatic in LLVM? No strong feelings either way though.

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

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5373
+            Builder.CreateIntrinsic(Intrinsic::x86_bitconvert_vector_to_tile,
+                                    {}, {ArgValue});
+        } else {
----------------
Something I don't really get is why we need the separate bitconvert intrinsic, and the existing cast intrinsic does not work. Do they have different semantics?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141899