This patch implements a DAG mutation which adds edges between different groups of instructions. The purpose is to try to generate code that conforms to a pipeline (groupA instructions occur before groupB, groupB -> groupC, and so on). Currently the pipeline order is hardcoded as VMEM->DSRead->MFMA->DSWrite, but the patch was designed to be easily extensible. Alias analysis is problematic for pipelining as memory instructions will usually not be able to be reordered w.r.t one another.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you name these files something so that it is clear that it is a DAG mutation involving MFMA? I'm also not sure pipeline is the correct terminology here? I know that library folks have been calling it that.
Will we eventually be able to specify the order of the pipeline types and the size of each InstructionClass cluster?
llvm/lib/Target/AMDGPU/AMDGPUSchedPipeline.cpp | ||
---|---|---|
34 ↗ | (On Diff #430755) | This should be abstracted by instruction type. |
44 ↗ | (On Diff #430755) | I'm not sure I understand what IsSized means? |
51 ↗ | (On Diff #430755) | Remove commented code. |
Addressed review comments:
- Added options for sizing of specific instruction groups
- Renamed the DAG Mutation -- renaming suggestions welcomed.
- Minor details
As a basis for our planned extensions, this looks OK.
In the future try to comment your code :)
llvm/lib/Target/AMDGPU/AMDGPUMFMAIGroupLP.cpp | ||
---|---|---|
36 | Can these options all have the amdgpu-mfma-igrouplp prefix? |
Addressed Review comments:
- Command line option naming.
- Included comments where code was potentially confusing
llvm/lib/Target/AMDGPU/AMDGPUMFMAIGroupLP.cpp | ||
---|---|---|
36 | Hey Austin -- thanks for the comments. Yep, that will be one of the main focuses of the next iteration. |
Testing on CK uncovered an edge case error w/ the way bundle iteration was done.
Experimentation on gemm_xdl_fp16 shows ~6% improvement from this mutation
Thanks! Good catch on a region that ends with a bundle. I think your last diff overwrote the changes before last. I.e. the added comments and renamed cl opts are gone.
llvm/lib/Target/AMDGPU/AMDGPUMFMAIGroupLP.cpp | ||
---|---|---|
26 | This is a bad name for this. DEBUG_TYPE pass names should be all lowercase for consistency. I'm also not a fan of not-a-pass debug prefixes. Can this use misched or whatever the actual scheduler's DEBUG_TYPE is? | |
172–173 | Check mayLoad first? | |
182 | Check mayStore first? |
This is a bad name for this. DEBUG_TYPE pass names should be all lowercase for consistency. I'm also not a fan of not-a-pass debug prefixes. Can this use misched or whatever the actual scheduler's DEBUG_TYPE is?