[PATCH] D154941: [mlir][ArmSME] Add custom get_tile_id and cast ops

[Diego Caballero via Phabricator via llvm-commits]

dcaballe accepted this revision.
dcaballe added a comment.
This revision is now accepted and ready to land.

LGTM! I like the abstraction! Awesome to see this moving forward!



================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:44
+class SMETileType<Type datatype, list<int> dims, string description>
+  : ShapedContainerType<[datatype],
+      And<[IsVectorOfRankPred<[2]>, allDimsScalableVectorTypePred,
----------------
Is there a construct to make this a VectorType instead of a ShapedType? I guess the subsequent predicates constraint the shaped type a bit more but it would be great if this could be an vector type directly.


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:118
+    individual vector ops, rather than "global" rewrites that would have to
+    look at the vector op uses and also lower them.
+
----------------
This abstraction sounds really great!


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:131
+def CastVectorToTile : ArmSME_Op<"cast_vector_to_tile", [Pure, TileElementWidthMatchesTileID]> {
+  let summary = "Cast from 2-d scalable vector type to tile id";
+  let description = [{
----------------
I guess you also considered introducing a single cast op that could cast both ways depending on the order of the operand/types. I think having two makes sense since this cast is kind of crossing two domains...


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:166
+def GetTileID : ArmSME_Op<"get_tile_id", [Pure]> {
+  let summary = "Returns an SME \"virtual tile\" id";
+  let description = [{
----------------
nit: do the quotes imply anything on `virtual tiles`? I think I don't get what it is :)


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

https://reviews.llvm.org/D154941