Skip to content

Conversation

@EnricoDeg
Copy link
Contributor

Proposed changes

Summary:

  • Add support for direct store in epilogue instead of cshuffle (performance improvement for small K problems)
  • Add padding support for wave transfer without transpose
  • Add wave transfer with interleaved layout to support direct store
  • Enable new functionalities on GEMMs
  • Add optional new functionality support for grouped convolution fwd
  • Add fast instances for grouped convolution fwd with new functionalities (performance improvements)

Checklist

Please put an x into the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.

  • I have added tests relevant to the introduced functionality, and the unit tests are passing locally
  • I have added the test to REGRESSION_TESTS list defined at the top of CMakeLists.txt in tests/CMakeLists.txt, IF the test takes more than 30 seconds to run.
  • I have added inline documentation which enables the maintainers with understanding the motivation
  • I have removed the stale documentation which is no longer relevant after this pull request
  • (If this change is user-facing) I have added release notes which provide the end users with a brief summary of the improvement from this pull request
  • I have run clang-format on all changed files
  • Any dependent changes have been merged

Discussion

If this is a relatively large or complex change, feel free to start a discussion by explaining why you chose the solution you did and what alternatives you considered

Copy link
Contributor

@kabrahamAMD kabrahamAMD left a comment

Choose a reason for hiding this comment

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

LGTM, good work

index_t,
index_t)
{
// Notes: padding is currently not supported
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment (or remove, as it provides no additional information to the error message)

@EnricoDeg EnricoDeg force-pushed the streamhpc/remove_cshuffle branch from 461a40d to 9134cbb Compare December 23, 2025 07:05

// Limitations of the current implementation:
// - no multiAB
// - GemmSpecialization Default
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like GemmSpec default is no longer a hard requirement for using WaveTransfer, but only when A B layouts are not row column respectively? If so update comment?


// We need to investigate if it makes sense to remove cshuffle for smaller types
// Currently we use direct store for NRepeat equal to 4 or 8. For 16 bit type we use at
// lease buffer store 64 bit for 16 contiguous threads -> 128 bytes in toral (full cache line)
Copy link
Contributor

Choose a reason for hiding this comment

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

typos: lease, toral

@krithalith
Copy link
Contributor

LGTM, had some small notes and I have a few questions:

  • Has conv fwd performance with wavetransfer been benchmarked?
  • Has wavetransfer / directstore been benchmarked in general?
  • If no to the above two questions, should we make a new issue for this?
  • Why do we only have wavetransfer instances for grouped conv fwd vanilla 2D?
  • Where did the vanilla fwd 2d instances come from?

@krithalith
Copy link
Contributor

LGTM, had some small notes and I have a few questions:

* Has conv fwd performance with wavetransfer been benchmarked?

* Has wavetransfer / directstore been benchmarked in general?

* If no to the above two questions, should we make a new issue for this?

* Why do we only have wavetransfer instances for grouped conv fwd vanilla 2D?

* Where did the vanilla fwd 2d instances come from?

Discussed offline. Basically benchmarks have been performed and wavetransfer did seem to be a decent bit better for 2D. No significant improvement for 3D so no instances added. We want to look into more automated benchmarks later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants