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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
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:
> c-rhodes wrote:
> > 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 :)
> > 
> > 
> > > 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 :)
> > 
> > 
> 
> To clarify, without this check:
> ```
> %tile = arm_sme.cast_tile_to_vector %tile_id : i32 to vector<[4]x[8]xi32>
> ```
> 
> would be valid
> To clarify, without this check:
> ```%tile = arm_sme.cast_tile_to_vector %tile_id : i32 to vector<[4]x[8]xi32>```
> would be valid

Could you double-check? This works fine:

```
module {
  func.func @arm_sme_cast_tile_to_vector_i8(%arg0: i8) -> vector<[4]x[16]xi8> {
    %0 = arm_sme.cast_tile_to_vector %arg0 : i8 to vector<[4]x[16]xi8>
    return %0 : vector<[4]x[16]xi8>
  }
}
```

You will need to replace `SMETile` with `AnyVectorOfAnyRank` in the definition of `CastTileToVector`, but `IsVectorOfShape` should trigger in both cases, right?


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

https://reviews.llvm.org/D154941