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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline 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,
----------------
awarzynski wrote:
> 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?
> How about [[ https://github.com/llvm/llvm-project/blob/60c9d2993bbf1594e89e1e6f72e1472eb1aeb8ef/mlir/include/mlir/IR/OpBase.td#L667-L669 | ScalableVectorOf ]]? Could that be applicable here?

All of the existing scalable vector predicates only check is scalable (i.e. any dim) not all dims scalable


================
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 = [{
----------------
awarzynski wrote:
> 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!
In the context of SME I think it's clear what a tile is, but I have no strong feelings either way.


================
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>;
----------------
awarzynski wrote:
> 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.
> I am just thinking that every vector that you create like this will satisfy this condition and to me this check feels redundant.

And this verifies that :)




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

https://reviews.llvm.org/D154941