[PATCH] D158586: [mlir][ArmSME] Lower vector.broadcast to ArmSME

[Andrzej Warzynski via Phabricator via llvm-commits]

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

LGTM, thanks!

I've made a few small suggestions, but it's up to you whether to incorporate them. Great to see all this "just working" :)



================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:29
+/// the tile slice index.
+static scf::ForOp generateTileSliceLoop(PatternRewriter &rewriter, Location loc,
+                                        Type eltType) {
----------------
[nit] `getLoopOverTileSlices`?


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:45
+
+// Returns a tile of the given vector type.
+static Value getTile(PatternRewriter &rewriter, Location loc, VectorType type) {
----------------
> // Returns a tile of the given vector type.

Could this be more specific? Perhaps rename as `getSMETileAndCastToVector`. It's a bit of a mouthful, but I wouldn't mind. Otherwise, a longer comment would be appreciated :)

[nit] Use doxygen


================
Comment at: mlir/lib/Conversion/VectorToArmSME/VectorToArmSME.cpp:172
 
+/// Conversion pattern for vector.broadcast.
+struct BroadcastOpToArmSMELowering
----------------
I find such short sample snippets very helpful. I know that being accurate can be painful (and lead to unnecessarily long comments), but its not a doc test, so it's OK to focus on the structure and skip the details.


================
Comment at: mlir/test/Dialect/ArmSME/vector-ops-to-sme.mlir:186
+// CHECK-SAME:                                          %[[SRC:.*]]: vector<f32>) {
+// CHECK: %[[SRC_1D:.*]] = vector.broadcast %[[SRC]] : vector<f32> to vector<[4]xf32>
+// CHECK: arm_sme.vector_to_tile_slice %[[SRC_1D]], {{.*}}
----------------
I'd be tempted to add (here and below):
```
// CHECK: scf.for
```

Just to highlight that that remains an important part of the lowering (no matter what's being broadcast).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158586