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

[HOTFIX][SWDEV-326460] check MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC in IsApplicable() #1451

Merged
merged 2 commits into from
Mar 8, 2022

Conversation

carlushuang
Copy link
Contributor

This is an hot fix for SWDEV-316705
Due to our current design, if a solver has multiple configs, and some of the config support deterministic(e.g. non-deterministic), some are not, we can not disable a single config in IsValid() function. But We need disable it in IsApplicable(), which means will disable this solver entirely if MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC env var is set.
This may have a performance impact, but since currently this env var is for test purpose (in this case performance impact is OK per our customer), so this approach is simple and fine.
If in future has performance requirement under MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC, need to seek for other approach and implementation.

@carlushuang
Copy link
Contributor Author

carlushuang commented Mar 6, 2022

blocked by #1452, CI will not pass

Update: fixed

@codecov

This comment was marked as off-topic.

Copy link
Contributor

@shaojiewang shaojiewang left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for CI.

@atamazov
Copy link
Contributor

atamazov commented Mar 8, 2022

@carlushuang I am Ok with this PR, but can you please comment out was is wrong with #1377?

Due to our current design, if a solver has multiple configs, and some of the config support deterministic(e.g. non-deterministic), some are not, we can not disable a single config in IsValid() function.

We can, and the library will use the default tuning config instead. The cost is a warning message.

@junliume
Copy link
Contributor

junliume commented Mar 8, 2022

@JehandadKhan @asroy @zjing14 @atamazov last call for reviewers :) Plan to merge and cherry pick later today 03/08.

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.

I am OK with this provided that all the affected iGemm solvers use floating-point addition in order to compute sums of values calculated by different work-items.

@atamazov
Copy link
Contributor

atamazov commented Mar 8, 2022

I highly recommend renaming this PR in order to include "(SWDEV-316705)" into commit name.

@junliume junliume changed the title [HOTFIX] check MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC in IsApplicable() [HOTFIX][SWDEV-326460] check MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC in IsApplicable() Mar 8, 2022
@junliume junliume merged commit fa2d589 into develop Mar 8, 2022
junliume added a commit that referenced this pull request Mar 8, 2022
…n IsApplicable() (#1451)

* check MIOPEN_DEBUG_CONVOLUTION_DETERMINISTIC in IsApplicable()

Co-authored-by: Jun Liu <[email protected]>
@junliume junliume deleted the hot_fix_deterministic_in_is_applicable branch April 14, 2022 17:18
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