[PATCH] D156467: [mlir][ArmSME] Add conversion from ArmSME to SCF to materialize loops

[Diego Caballero via Phabricator via llvm-commits]

dcaballe accepted this revision.
dcaballe added a comment.
This revision is now accepted and ready to land.

LGTM! Just a few comments. Thanks for addressing this so quickly!



================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:317-318
+  let description = [{
+    Loads a 1D tile slice from memory into a 2D SME "virtual tile". The tile
+    slice is defined by the 2D scalable vector type of the input tile. A tile
+    slice index describes where in the input tile the tile slice is loaded to.
----------------
I don't follow here. The tile slice (1D) is defined by the 2D type? Do you mean by the dimension of the 2D type pointed by the index?


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:322
+
+    The memory is defined by a base and index and must be contiguous. The
+    memref must be either rank 1 or rank 2 with dynamic dimensions, since the
----------------
-> ~The slice of memory read is defined...?


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:329
+    ```mlir
+    %tile_update = arm_sme.tile_slice_load_and_update %base[%c0], %tile, %tile_slice_index : memref<?x?xi8>, vector<[16]x[16]xi8>
+    ```
----------------
This looks like a vector load with a passthru value so I would remove the `_and_update` part to make it shorter but up to you :)


================
Comment at: mlir/lib/Conversion/ArmSMEToSCF/ArmSMEToSCF.cpp:84
+
+    rewriter.setInsertionPointToStart(forOp.getBody());
+
----------------
Add a builder guard at the beginning of the function to restore the insertion point


================
Comment at: mlir/lib/Conversion/ArmSMEToSCF/ArmSMEToSCF.cpp:148
+
+    rewriter.setInsertionPointToStart(forOp.getBody());
+
----------------
same


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156467