[PATCH] D157005: [mlir][ArmSME] Add vector to tile slice op and lowerings

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:417
 
+def VectorToTileSliceOp : ArmSME_Op<"vector_to_tile_slice", [
+    AllTypesMatch<["tile", "result"]>
----------------
awarzynski wrote:
> Most Ops' names convey what the corresponding "action"/"function" is. Wondering whether this shouldn't be `MoveVectorToTileSliceOp` instead. Naming is hard.
> Most Ops' names convey what the corresponding "action"/"function" is. Wondering whether this shouldn't be `MoveVectorToTileSliceOp` instead. Naming is hard.

are you suggesting to rename it both internally and externally (i.e. `move_vector_to_tile_slice`) or just the former?


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:424
+    (2-D scalable vector) slice at the given index. The 1-D vector type must
+    match the inner vector type of the 2-D vector type that represents the
+    tile. The updated tile is returned as the result.
----------------
dcaballe wrote:
> -> element type?
> -> element type?

For a 2-d scalable vector such as `vector<[4]x[4]xi32>` is the element type `vector<[4]xi32>`? That's what I want to express here but I do struggle with these descriptions. FWIW `getElementType()` returns the scalar `i32`. Perhaps `The 1-D vector type must match the vector type of the inner dimension of the 2-D vector type`?


================
Comment at: mlir/test/Integration/Dialect/Vector/CPU/ArmSME/tile_fill.mlir:74
+
+    // TODO: this can be replaced with vector.print when D156519 lands.
+    llvm.call @printOpen() : () -> ()
----------------
benmxwl-arm wrote:
> It's landed :) 
> 
> It's landed :) 
> 

Thanks for heads up I'll update this



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157005