Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double buffering improvements #1511

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

giuseros
Copy link
Contributor

  • Split the LDS reads and MFMA/WMMA into two independent loops
  • Have them into two separate stages (so that they can be executed in parallel)

This is to make our pipeline similar to what CK is doing in:

@giuseros giuseros requested review from jerryyin and sjw36 as code owners May 13, 2024 10:28
- Split the LDS reads and MFMA/WMMA into two independent loops
- Have them into two separate stages (so that they can be executed in
parallel)

This is to make our pipeline similar to what CK is doing in:
- https://github.com/ROCm/composable_kernel/blob/6d073d31bbc7d39d8b170d549f2af61970378150/include/ck/tensor_operation/gpu/block/blockwise_gemm_pipeline_xdlops_v4.hpp
@giuseros giuseros force-pushed the reschedule-loops branch from 6c301ff to c9930bc Compare May 13, 2024 15:17
Copy link
Collaborator

@krzysz00 krzysz00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this make BlockwiseGemm kinda obselete?

Copy link
Collaborator

@krzysz00 krzysz00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're comfortable leaving dead code running around, I don't see any deep reason not to land this.

Since it doesn't look like it makes sense to e2e test this, can we get a lit test for this new structure? Maybe some sort of internal forceDoubleBufferForTesting attribute

@giuseros
Copy link
Contributor Author

giuseros commented May 17, 2024

Re: BlockwiseGemm: I don't think so, because in BlockwiseGemm the intuition is that we do everything at the same time
Re: DeadCode: it depends. We have introduced other things (like multi-buffers) that due to backend/hw limitations cannot be used (yet). I see this as progressively having a better scheduling, so we need to cope with some unused code until we completely sort this out. On the other hand, we can surely add an option where we enable all this and test it

@krzysz00
Copy link
Collaborator

Re BlockwiseGemm, yeah, I misread the comment

Re testing ... yeah, I'd like a lit test capturing the code structure you're expecting in the double-buffering case after gridwise-gemm-to-blockwise

@manupak
Copy link
Contributor

manupak commented May 22, 2024

This is fine with me for now as I had this long standing opinion, the interface for BlockwiseGemm is too limiting -- the fact that it requires LDS buffers to be passed in as inputs.

Long-term :

What would be better (as most other compilers IREE, Triton) is we do BlockwiseGemm take register input with the layout (i.e. tid, iter --> blockwise_tensor). Then let the lowering decides whether it needs to swizzle stuff with whatever way (DPP, LDS, etc) is suitable. We could even define a op : blockwise_copy to encapsulate layout changes.

In the case of user-defined buffering (i.e. what we currently do in gridwise lowering of gemm), user is free to also load directly into the appropiate layout a BlockwiseGemm would prefer. So this would strictly limit the BlockwiseGemm to be just "compute" part of the split you are doing here.

@manupak
Copy link
Contributor

manupak commented Sep 17, 2024

@giuseros should we close this ? I dont think this is happening. correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants