[PATCH] D154955: [mlir][ArmSME] Implement tile allocation

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks for working on this! The overall logic makes a lot of sense to me.

My only concern is that "tile allocation" could be confused with "tiling" in e.g. Linalg (I'm sure there's more examples in MLIR). I suggest making it a bit clearer that this is "SME virtual tile allocation" (or "tile allocation for virtual SME tiles"). Even in the context of SME, we will be using Linalg tiling :) I made some specific suggestions inline. You could try even some bolder changes, e.g. "TileAllocationPass" --> "SMEVirtualTileAllocationPass" (or something similar). This is a nice-to-have.



================
Comment at: mlir/include/mlir/Dialect/ArmSME/Transforms/Passes.td:43
+def TileAllocation
+    : Pass<"allocate-sme-tiles", "mlir::func::FuncOp"> {
+  let summary = "Allocate SME tiles";
----------------
Looks like we use `arm-` for pretty much everything Arm-related, so suggest adding it here for consistency.


================
Comment at: mlir/include/mlir/Dialect/ArmSME/Transforms/Passes.td:46-48
+    This pass does tile allocation at the 'func.func' op level, replacing
+    'arm_sme.get_tile_id' with actual tiles. An error will currently be emitted
+    when there's no tiles left.
----------------
Just suggested to refine/expand a bit.

I would drop "currently" as there are no timescales (yet) for relaxing that.


================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp:63
+
+static constexpr char kTilesInUse[] = "tilesInUse";
+
----------------
[nit] I would rename this to something a bit more specific. Also, isn't the convention to use underscores? So, e.g. `arm_sme_tiles_in_use`?


================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp:152
+  }
+  return tileIDOp.emitError("ran out of tiles!");
+}
----------------
I suggest making this error a bit more specific so that it's easy to grep for. I appreciate that this is not ambiguous when only one pass is run. But this will eventually be embedded in some larger compilation pipelines that will include tiling :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154955