This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ArmSME] Implement tile allocation
ClosedPublic

Authored by c-rhodes on Jul 11 2023, 5:49 AM.

Details

Summary

This patch adds a pass '-allocate-sme-tiles' to the ArmSME dialect that
implements allocation of SME ZA tiles.

It does this at the 'func.func' op level by replacing
'arm_sme.get_tile_id' ops with 'arith.constant' ops that represent the
tile number. The tiles in use in a given function are tracked by an
integer function attribute 'tilesInUse' that is a 16-bit tile mask with
a bit for each 128-bit element tile (ZA0.Q-ZA15.Q), the smallest ZA tile
granule. 'tilesInUse' is initialized on the first 'arm_sme.get_tile_id'
rewrite and updated on each subsequent rewrite. Mixing of different
element tile types is supported.

Section B2.3.2 of the SME spec [1] describes how the 128-bit element
tiles overlap with other element tiles.

Depends on D154941

[1] https://developer.arm.com/documentation/ddi0616/aa

Diff Detail

Event Timeline

c-rhodes created this revision.Jul 11 2023, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 5:49 AM
c-rhodes requested review of this revision.Jul 11 2023, 5:49 AM

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.

mlir/include/mlir/Dialect/ArmSME/Transforms/Passes.td
43

Looks like we use arm- for pretty much everything Arm-related, so suggest adding it here for consistency.

46–48

Just suggested to refine/expand a bit.

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

mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp
64

[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?

153

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 :)

c-rhodes updated this revision to Diff 539594.Jul 12 2023, 9:14 AM

Address comments

c-rhodes marked 4 inline comments as done.Jul 12 2023, 9:18 AM

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.

mlir/include/mlir/Dialect/ArmSME/Transforms/Passes.td
43

Looks like we use arm- for pretty much everything Arm-related, so suggest adding it here for consistency.

Good spot!

mlir/lib/Dialect/ArmSME/Transforms/TileAllocation.cpp
153

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.

awarzynski accepted this revision.Jul 13 2023, 4:38 AM

Cheers for addressing my comments, LGTM!

This revision is now accepted and ready to land.Jul 13 2023, 4:38 AM
c-rhodes updated this revision to Diff 540440.Jul 14 2023, 8:55 AM
c-rhodes marked 2 inline comments as done.
  • Scalar int type of arith.constant should match that of the arm_get.get_tile_id it's replacing.
  • Add -allocate-arm-sme-tiles to mlir/test/Integration/Dialect/Vector/CPU/ArmSME/vector-ops.mlir.
Matt added a subscriber: Matt.Jul 14 2023, 2:45 PM
This revision was automatically updated to reflect the committed changes.