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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM, thanks for addressing my comments :)



================
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,
----------------
c-rhodes wrote:
> dcaballe wrote:
> > 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.
> > 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.
> 
> I don't believe there is an existing one, the vector ones in `mlir/include/mlir/IR/OpBase.td` I used for reference here also use this, but perhaps `ShapedContainerType` could be copied for a `VectorType`.
How about [[ https://github.com/llvm/llvm-project/blob/60c9d2993bbf1594e89e1e6f72e1472eb1aeb8ef/mlir/include/mlir/IR/OpBase.td#L667-L669 | ScalableVectorOf ]]? Could that be applicable here?


================
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 = [{
----------------
c-rhodes wrote:
> dcaballe wrote:
> > nit: do the quotes imply anything on `virtual tiles`? I think I don't get what it is :)
> > nit: do the quotes imply anything on `virtual tiles`? I think I don't get what it is :)
> 
> To be honest I would prefer we just use `tile`, but the rationale is that these are not real tiles but merely “views” into ZA.
I've been suggesting "virtual tiles" as different people mean different things when referring to tiles. "SME virtual tiles" is just a way to highlight that:
* We mean the tiles in the context of the Arm SME extension (as opposed to e.g. tiles when tiling a matmul).
* These tiles are actually "views" into ZA rather than "tiles". A "tile" to me suggests that it's something "square" and so "ZA tile" could, incorrectly, imply "a square section of ZA". 

It's a name that one of our architects at Arm has been using and I feel that's very fitting. Naming is hard!


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:46-47
+      And<[IsVectorOfRankPred<[2]>,
+           CPred<[{::llvm::cast<::mlir::VectorType>($_self).allDimsScalable()}]>,
+           CPred<"::llvm::cast<::mlir::VectorType>($_self).getShape() == ArrayRef<int64_t>({" # !interleave(dims, ", ") # "})">]>,
+  description>;
----------------
c-rhodes wrote:
> awarzynski wrote:
> > [nit] Perhaps extract the preconditions to dedicated definition?
> > 
> > Also, when would this be triggered:
> > ```
> >  CPred<"::llvm::cast<::mlir::VectorType>($_self).getShape() == ArrayRef<int64_t>({" # !interleave(dims, ", ") # "})"
> > ```
> > It feels like a "VectorType verifier" that could be safely skipped (i.e. nothing SME specific).
> > Also, when would this be triggered:
> > ```
> >  CPred<"::llvm::cast<::mlir::VectorType>($_self).getShape() == ArrayRef<int64_t>({" # !interleave(dims, ", ") # "})"
> > ```
> > It feels like a "VectorType verifier" that could be safely skipped (i.e. nothing SME specific).
> 
> Please could you clarify, not sure what you mean? This verifies the shape, i.e. `vector<[16]x[16]xi8>` is (16, 16).
I am just thinking that every vector that you create like this will satisfy this condition and to me this check feels redundant. But I am probably just failing to understand the underlying rationale. No harm in keeping this.


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

https://reviews.llvm.org/D154941