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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.
Herald added a subscriber: sunshaoce.

A few minor points/questions. LG otherwise



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


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:106
+
+    // Lower 'arith.constant dense<0>' to 'arm_sme.zero' op.
+    if (isSplatZero(tileElementType, denseAttr)) {
----------------
[nit] Just to make the split into 2 cases more visible.


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:112
+
+    // Lower non-zero constants to a loop of 'arm_sme.vector_to_tile_slice' ops
+    // that broadcast the constant to each tile slice.
----------------
[nit] Just to make the split into 2 cases more visible.


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:137
+
+    // Create a loop that loads each ZA tile slice from memory.
+    auto step = rewriter.create<arith::ConstantIndexOp>(loc, 1);
----------------
IIUC, the following block will only create the loop structure. The following operation is not created here:

> loads each ZA tile slice from memory.

That's created further down. Also, rather than "loading from memory", this is creating "move from vector to a slice", right?


================
Comment at: mlir/test/Dialect/ArmSME/roundtrip.mlir:586
+  %c0 = arith.constant 0 : index
+  arm_sme.vector_to_tile_slice %vector, %tile, %tile_slice_index : vector<[16]xi8> into vector<[16]x[16]xi8>
+  return
----------------
Could you add some invalid cases in "invalid.mlir"?


================
Comment at: mlir/test/Dialect/ArmSME/vector-ops-to-sme.mlir:251
+
+// CHECK-LABEL: func.func @arith_constant_dense_2d_nonzero_i8() {
+// CHECK: %[[C2_SPLAT:.*]] = arith.constant dense<2> : vector<[16]xi8>
----------------
It would be good to check at least one more element type.


================
Comment at: mlir/test/Integration/Dialect/Vector/CPU/ArmSME/tile_fill.mlir:54
+
+  // Fill a tile with '1'. This will get lowered to a 1-d vector splat of '1'
+  // and a loop that writes this vector to each tile slice in the ZA tile.
----------------
[nit] I would use some other value - 1 is super common and can be easily missed. Here it would be nice to emphasise that it could be _anything_.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157005