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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 4 inline comments as done.
c-rhodes added a comment.

In D154955#4493675 <https://reviews.llvm.org/D154955#4493675>, @awarzynski wrote:

> 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.

Thanks for the comments! I'll keep it as TileAllocation for simplicity but we can rename it in the future if it's causing confusion within the dialect.



================
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";
----------------
awarzynski wrote:
> Looks like we use `arm-` for pretty much everything Arm-related, so suggest adding it here for consistency.
> Looks like we use `arm-` for pretty much everything Arm-related, so suggest adding it here for consistency.

Good spot!


================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp:152
+  }
+  return tileIDOp.emitError("ran out of tiles!");
+}
----------------
awarzynski wrote:
> 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 :)
> 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 :)

Updated, but just wanted to note this error is on the `arm_sme.get_tile_id` op so confusion with tiling seems unlikely.


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

https://reviews.llvm.org/D154955