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

[OCL][MI100][MI200] Fix iGemm ASM GTC XDLOPS failures with OCL backend (Staging 95b58f72f) (#1317) and Implement abstraction for multi-buffer workspace (#1326) #1327

Merged
merged 24 commits into from
Dec 29, 2021
Merged

[OCL][MI100][MI200] Fix iGemm ASM GTC XDLOPS failures with OCL backend (Staging 95b58f72f) (#1317) and Implement abstraction for multi-buffer workspace (#1326) #1327

merged 24 commits into from
Dec 29, 2021

Conversation

shaojiewang
Copy link
Contributor

@shaojiewang shaojiewang commented Dec 7, 2021 •
edited
Loading

  1. fix Implement and use an abstraction that represents a multi-buffer workspace #1326 : abstract a class to hold the offsets and sizes of each sub buffer for transpose+asm_igemm solvers.
  2. fix [OCL][MI100][MI200] iGemm ASM GTC XDLOPS failures with OCL backend (Staging 95b58f72f) #1317 : fix mem fault on OCL BE
  3. add some test case for transpose+asm_igemm solvers.

@codecov

This comment has been minimized.

Sign in to view
@shaojiewang
Copy link
Contributor Author

shaojiewang commented Dec 7, 2021 •
edited
Loading

@atamazov I can not run ctest on gfx90a with OCL BE at all. Could you please help to check if some code work not properly?

@atamazov
Copy link
Contributor

atamazov commented Dec 7, 2021

@shaojiewang Yes, as soon I do some other urgent work.

@junliume junliume changed the title Fix issue#1317 #1326 [OCL][MI100][MI200] Fix iGemm ASM GTC XDLOPS failures with OCL backend (Staging 95b58f72f) (#1317) and Implement abstraction for multi-buffer workspace (#1326) Dec 9, 2021
@junliume
Copy link
Collaborator

@atamazov @shaojiewang should we include this into the next round of staging too? Is it related to
SWDEV-313026 ?

atamazov
atamazov previously requested changes Dec 12, 2021
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

This should remove WORKAROUND_ISSUE_1317 from #1321

@atamazov
Copy link
Contributor

@junliume No, this is not needed in 5.0.

@shaojiewang
Copy link
Contributor Author

This should remove WORKAROUND_ISSUE_1317 from #1321

@atamazov The W/A macro was removed.

@atamazov
Copy link
Contributor

Oops, looked into wrong diff.

@atamazov atamazov dismissed their stale review December 13, 2021 02:12

Incorrect review

@junliume junliume dismissed atamazov’s stale review December 17, 2021 02:06

Review recommendations adopted. Please re-review.

@atamazov
Copy link
Contributor

@JehandadKhan need_find_db_update because this increases required workspace size for the affected solvers.

@atamazov atamazov mentioned this pull request Dec 17, 2021
Closed
atamazov
atamazov previously requested changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

This can't be merged as is due to #1349

@atamazov
Copy link
Contributor

@shaojiewang #1352 (which extends WORKAROUND_ISSUE_1317 to tests) will be merged soon. This PR should revert it.

@shaojiewang
Copy link
Contributor Author

@shaojiewang #1352 (which extends WORKAROUND_ISSUE_1317 to tests) will be merged soon. This PR should revert it.

OK

@junliume junliume dismissed atamazov’s stale review December 21, 2021 00:37

#1352 merged, now we can move forward with this one. Please re-review

@atamazov atamazov removed the ON_HOLD label Dec 22, 2021
@junliume
Copy link
Collaborator

@shaojiewang tried to resolve conflict, please double check :)

@shaojiewang
Copy link
Contributor Author

@shaojiewang tried to resolve conflict, please double check :)

Thanks a lot, I'll check very soon.

@shaojiewang
Copy link
Contributor Author

@junliume error removed.
@atamazov #1352 is reverted.

@atamazov
Copy link
Contributor

This can't be merged as is due to #1349

#1349 is closed, so this can be merged now.

atamazov
atamazov approved these changes Dec 28, 2021
View reviewed changes
Copy link
Contributor

@atamazov atamazov left a comment

Choose a reason for hiding this comment

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

LGTM

@atamazov
Copy link
Contributor

@shaojiewang Please do not use hash sign in branch names anymore. It seems incompatible with some functions of GUI of our CI.

@shaojiewang
Copy link
Contributor Author

Please do not use hash sign in branch names anymore. It seems incompatible with some functions of GUI of our CI.

Yes, sure. Sorry for that.

@atamazov
Copy link
Contributor

@shaojiewang

Yes, sure. Sorry for that.

No problem ;)

@atamazov atamazov merged commit 7416ce6 into develop Dec 29, 2021
@junliume junliume deleted the fix_issue#1317_#1326 branch January 7, 2022 18:30
@atamazov atamazov mentioned this pull request Feb 23, 2022
Closed
@aska-0096 aska-0096 mentioned this pull request Mar 25, 2022
Merged
@atamazov atamazov mentioned this pull request Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atamazov atamazov atamazov approved these changes

@carlushuang carlushuang Awaiting requested review from carlushuang

@junliume junliume Awaiting requested review from junliume junliume is a code owner

@shurale-nkn shurale-nkn Awaiting requested review from shurale-nkn

Assignees
No one assigned
Labels
IMPACTS_FIND_DB TESTING_CI_PASSED urgency_normal
Projects
None yet
Milestone
No milestone
3 participants
@shaojiewang @atamazov @junliume