Page MenuHomePhabricator

[AMDGPU] Instruction Type Pipeline
ClosedPublic

Authored by jrbyrnes on May 19 2022, 11:31 AM.

Details

Summary

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.

Diff Detail

Event Timeline

jrbyrnes created this revision.May 19 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:31 AM
jrbyrnes requested review of this revision.May 19 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:31 AM

Add [AMDGPU] to the title.

jrbyrnes retitled this revision from Instruction Type Pipeline to [AMDGPU] Instruction Type Pipeline.May 19 2022, 11:51 AM

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.

jrbyrnes updated this revision to Diff 431454.EditedMay 23 2022, 12:36 PM

Addressed review comments:

  1. Added options for sizing of specific instruction groups
  2. Renamed the DAG Mutation -- renaming suggestions welcomed.
  3. Minor details
kerbowa accepted this revision.May 24 2022, 10:53 PM

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?
In the next iteration of the patch these will be combined correct?

This revision is now accepted and ready to land.May 24 2022, 10:53 PM
jrbyrnes updated this revision to Diff 432032.May 25 2022, 9:52 AM
jrbyrnes marked 3 inline comments as done.

Addressed Review comments:

  1. Command line option naming.
  2. Included comments where code was potentially confusing
jrbyrnes marked an inline comment as done.May 25 2022, 9:58 AM
jrbyrnes added inline comments.
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.

jrbyrnes updated this revision to Diff 432422.May 26 2022, 4:59 PM
jrbyrnes marked an inline comment as done.

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.

jrbyrnes updated this revision to Diff 432424.May 26 2022, 5:19 PM

Write back the changes that were accidentally overwritten

I think your last diff overwrote the changes before last. I.e. the added comments and renamed cl opts are gone.

Thanks for pointing this out!

kerbowa accepted this revision.May 31 2022, 10:43 AM
This revision was landed with ongoing or failed builds.May 31 2022, 10:52 AM
This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Jun 1 2022, 5:23 AM
llvm/lib/Target/AMDGPU/AMDGPUMFMAIGroupLP.cpp
25

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?

171–172

Check mayLoad first?

181

Check mayStore first?

antc added a subscriber: antc.Jun 2 2022, 2:39 AM