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

[Tuning] Remove Solver ConvHipImplicitGemmBwdDataV4R1Xdlops FP32 #1209

Merged
merged 2 commits into from
Oct 6, 2021

Conversation

cderb
Copy link
Contributor

@cderb cderb commented Oct 5, 2021

Removes FP32 ConvHipImplicitGemmBwdDataV4R1Xdlops from find_db replacing with second best Backward Data Implicit GEMM solver.

@cderb cderb added the tuning label Oct 5, 2021
@cderb cderb requested review from JehandadKhan and atamazov October 5, 2021 18:18
@junliume
Copy link
Contributor

junliume commented Oct 5, 2021

@cderb Thanks! Let me try to target 'Fp16 Hip All Install gfx908" stage and see if it passed there.

@junliume
Copy link
Contributor

junliume commented Oct 6, 2021

@cderb Thanks! Let me try to target 'Fp16 Hip All Install gfx908" stage and see if it passed there.

Platform-specific tests all passed

@junliume junliume merged commit 12e52ed into develop Oct 6, 2021
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.

Let's avoid rush ;)

@@ -30,10 +30,6 @@
#include <miopen/solver/implicitgemm_util.hpp>
#include <cstddef>

/// Disable ConvHipImplicitGemmBwdDataV4R1Xdlops by default.
Copy link
Contributor

@atamazov atamazov Oct 6, 2021

Choose a reason for hiding this comment

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

This W/A must be not removed but narrowed to FP32 in this PR at least. Otherwise the next tuning round will bring the problem back. And I am not sure that ConvHipImplicitGemmBwdDataV4R1Xdlops is good for FP16.

test_regression_half_mi100 has failed because the failing test case is intended for ConvHipImplicitGemmBwdDataV4R1Xdlops specifically. This test should have been disabled in #1207 (and in #1208) under the WORKAROUND_ISSUE_1206

Copy link
Contributor

Choose a reason for hiding this comment

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

Another reason for keeping the W/A is that the end user can activate the Normal Find mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fdb update is good as it recovers some performance.

@atamazov
Copy link
Contributor

atamazov commented Oct 6, 2021

This PR wasn't squashed, and git history does not show (#1209).

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.

3 participants