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

[HOTFIX] disable bwd_gemm solver to fix 2474 #2477

Closed
wants to merge 4 commits into from
Closed

[HOTFIX] disable bwd_gemm solver to fix 2474 #2477

wants to merge 4 commits into from

Conversation

carlushuang
Copy link
Contributor

No description provided.

This was referenced Oct 23, 2023
Closed
Closed
atamazov
atamazov suggested changes Oct 23, 2023
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.

  • It should be possible to cancel the W/A by explicit setting MIOPEN_DEBUG_CONV_GEMM=1

src/solver/gemm_bwd.cpp Outdated
@@ -43,6 +43,7 @@
MIOPEN_DECLARE_ENV_VAR(MIOPEN_CONV_PRECISE_ROCBLAS_TIMING)

#define WORKAROUND_MIOPENGEMM_ISSUE_59 1
#define WORKAROUND_MIOPENGEMM_ISSUE_2474 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define WORKAROUND_MIOPENGEMM_ISSUE_2474 1
#define WORKAROUND_ISSUE_2474 1

We do not use MIOpenGemm (GEMM backend for OpenCL) anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in fa5a598

Copy link
Contributor

Choose a reason for hiding this comment

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

[resolved]

@atamazov
Copy link
Contributor

@carlushuang @junleemoreh @JehandadKhan
Generally, this W/A seems too wide to be merged in, unless there is some absolute need. The promotion of MIOpen into Staging (amd-develop) should not be performed until W/A is removed.

I recommend redesigning:

  • Introduce the individual env controls for GEMM solvers.
  • The W/A should change the defaults of some of them (those that belong to backward suspects) from "enabled" to "disabled".
  • Explicit enabling of the env controls should re-enable the suspects.

@carlushuang
Copy link
Contributor Author

Hi @atamazov , this PR unblock the fix for issue: #2459, this is why I made a quick hotfix for the bwd_gemm solver

@atamazov
Copy link
Contributor

atamazov commented Oct 23, 2023 •
edited
Loading

@carlushuang

Hi @atamazov , this PR unblock the fix for issue: #2459, this is why I made a quick hotfix for the bwd_gemm solver

Do you see the problem on clean develop?

Regardless of the importance of this W/A the following remains valid:

  • The promotion of MIOpen into Staging (amd-develop) should not be performed until this W/A is removed or narrowed down.

(Just for awareness of everyone! 🤝 )

/cc @junliume @JehandadKhan

@carlushuang
Copy link
Contributor Author

This PR is identified as specific docker related issue, hence I will close this PR

@junliume junliume deleted the hotfix_bwd_gemm branch February 6, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atamazov atamazov atamazov requested changes

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

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

Assignees
No one assigned
Labels
ON_HOLD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carlushuang @atamazov @junliume