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.

Sign up for GitHub

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

Jump to bottom

Add ck mha fp16 solver #3277

Merged
merged 17 commits into from
Oct 4, 2024
Merged

Add ck mha fp16 solver #3277

merged 17 commits into from
Oct 4, 2024

Conversation

BrianHarrisonAMD
Copy link
Collaborator

Purpose of review:

  • Integrating CK FA V2 FWD inference solution into MIOpen.

Note:

  • Adding tests as a follow up PR.
  • This is being merged into an integration branch, and won't be merged into develop until tests are completed and merged as well.
  • This solver's functionality is limited to FWD inference for now.

@BrianHarrisonAMD BrianHarrisonAMD self-assigned this Sep 27, 2024
Vsevolod1983
Vsevolod1983 reviewed Oct 2, 2024
View reviewed changes
src/solution.cpp Outdated Show resolved Hide resolved
@Vsevolod1983 Vsevolod1983 self-requested a review October 2, 2024 14:49
@CAHEK7
Copy link
Contributor

CAHEK7 commented Oct 2, 2024

It would be nice to have some tests as well.

@BrianHarrisonAMD
Copy link
Collaborator Author

It would be nice to have some tests as well.

Absolutely!

Working on those now, and this will live in an integration branch until those are ready.

Vsevolod1983
Vsevolod1983 requested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@Vsevolod1983 Vsevolod1983 left a comment

Choose a reason for hiding this comment

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

changes requested related to ExecutePrimitive function call

@BrianHarrisonAMD
Code review comments
bd132b4 
- Update requirements to CK that generates MHA for gfx90a
- Fix compilation issue for No CK
- Use provided tensor strides instead of calculating
averinevg
averinevg reviewed Oct 3, 2024
View reviewed changes
src/solver.cpp Outdated Show resolved Hide resolved
@BrianHarrisonAMD BrianHarrisonAMD requested a review from a team as a code owner October 3, 2024 14:26
BrianHarrisonAMD
BrianHarrisonAMD commented Oct 3, 2024
View reviewed changes
requirements.txt
ROCm/composable_kernel@9c0811f39a2262dbe4d71b81898187951c1e11ba -DCMAKE_BUILD_TYPE=Release -DINSTANCES_ONLY=ON
ROCm/composable_kernel@294cb823142a815170cf1faa63e01a431a557a04 -DCMAKE_BUILD_TYPE=Release -DINSTANCES_ONLY=ON
Copy link
Collaborator Author

@BrianHarrisonAMD BrianHarrisonAMD Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note, this is pointing to a dev commit where I added generating the device_mha_operations lib for gfx90a as well.
This will need to be updated to point to an amd-develop commit instead once it's been promoted.

Since this is living in a integration branch it should be fine for now, but we shouldn't merge the integration into develop until this is resolved.

@BrianHarrisonAMD BrianHarrisonAMD merged commit 95186b8 into bharriso/integrate-ck-mha-fp16 Oct 4, 2024
140 checks passed
@BrianHarrisonAMD BrianHarrisonAMD deleted the bharriso/add-ck-mha-fp16-solver branch October 4, 2024 14:10
This was referenced Oct 8, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@averinevg averinevg averinevg left review comments

@junliume junliume junliume left review comments

@Vsevolod1983 Vsevolod1983 Vsevolod1983 approved these changes

@CAHEK7 CAHEK7 CAHEK7 approved these changes

@scerzh scerzh Awaiting requested review from scerzh

@bghimireamd bghimireamd Awaiting requested review from bghimireamd

@JehandadKhan JehandadKhan Awaiting requested review from JehandadKhan

Assignees

@BrianHarrisonAMD BrianHarrisonAMD

Labels
complexity_middle urgency_normal value_middle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BrianHarrisonAMD @CAHEK7 @Vsevolod1983 @junliume @averinevg