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

[NFC] Replace miopen::ProblemDescription with conv::ProblemDescription, part 2 #2148

Conversation

averinevg
Copy link
Collaborator

@averinevg averinevg commented May 12, 2023

List of changes:

  • ProblemDescription data fields have been removed
  • mlo_construct_direct2D_fusion has been replaced with ProblemDescription and removed
  • ProblemDescriptionCompatTemporary has been created for use in legacy code
  • Unused code has been removed

Blocker: ROCm/MIFin#83

See #266

…th ProblemDescriptionCompat for SQLitePerfDb and test_sqlite_perfdb
Jenkinsfile Outdated Show resolved Hide resolved
DrizztDoUrden
DrizztDoUrden previously approved these changes Jun 1, 2023
Copy link
Contributor

@DrizztDoUrden DrizztDoUrden left a comment

Choose a reason for hiding this comment

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

lgtm

DrizztDoUrden
DrizztDoUrden previously approved these changes Jun 2, 2023
Copy link
Contributor

@DrizztDoUrden DrizztDoUrden left a comment

Choose a reason for hiding this comment

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

lgtm

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!

@@ -95,7 +95,7 @@ static Random& Rnd()

struct ProblemData : SQLiteSerializable<ProblemData>
{
ProblemDescription prob;
ProblemDescriptionCompatTemporary prob;
Copy link
Contributor

Choose a reason for hiding this comment

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

[Future] This leftover must be removed with high urgency.

@atamazov
Copy link
Contributor

Let me run my set of performance tests before merging

@averinevg
Copy link
Collaborator Author

Today we discussed this PR with @atamazov. All validation tests passed, so we can merge the PR. Estimated time for performance test results - the day after tomorrow

@junliume junliume merged commit fd9eff7 into develop Jun 20, 2023
@atamazov
Copy link
Contributor

@averinevg @junliume

🌀 Performance testing results

Preconditions:

  • Radeon VII (gfx906/60)
  • ROCm 5.4.1, Ubuntu 20.04.5
  • BUILD_DEV=On, Release build
  • branch 18d368c vs. develop ad809f9
  • 93 known FP32 convolution configs
  • 93 known FP16 convolution configs

Tested modes:

  • (0) Immediate mode with default (system) find-db
  • (1) Find Mode (DYNAMIC_HYBRID)
  • (2) Find Mode (NORMAL)
  • (3) Immediate mode with user find-db filled by step (2).

Bottom line: 🟢 No performance or correctness regressions.

Logs & csv files available upon request.

@averinevg averinevg deleted the ea_replace_problem_description_with_conv_problem_description_part_2 branch July 25, 2023 11:11
Comment on lines -107 to -109
: ProblemDescription(dir == conv::Direction::Forward
? conv::ProblemDescription{in, weights, out, conv, dir, bias_}
: conv::ProblemDescription{out, weights, in, conv, dir, bias_})
Copy link
Contributor

Choose a reason for hiding this comment

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

After this PR, we do not swap xDesc and yDesc in the ProblemDescription, i.e. GetIn() always returns xDesc.

Copy link
Contributor

@atamazov atamazov Oct 25, 2023

Choose a reason for hiding this comment

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

Oh no, we still swap descriptors.

/cc @averinevg

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