conv::ProblemDescription vs miopen::ProblemDescription

[JehandadKhan]

The following lines of code in miopen::conv::ProblemDescription appear to be for the consumption of SQLite Perf DB

https://github.com/ROCmSoftwarePlatform/MIOpen/blob/e65e04ac1df4fa1381cf6ea439ad22b29998df42/src/include/miopen/conv/problem_description.hpp#L285

However, AFAIU only the Visit methods in miopen::ProblemDescription are used there. Furthermore, miopen::conv::ProblemDescription has a table_name() method, which is not used either.

Should we get rid of one of these code blocks, whichever is redundant. Can someone help me understand the design here?

@atamazov @DrizztDoUrden

[atamazov]

The second ProblemDescription was introduced in PR #2399. Some explanation at https://github.com/AMDComputeLibraries/MLOpen/pull/2399#issue-371438857.

Unfortunately, normal work on remaking the design was interrupted by a more "important" jobs. Let me omit details.

@DrizztDoUrden Please provide guidelines for programmers (which one of two is better ot use and when). Please also shed some light on you plans regarding this, if you can.

[atamazov]

[Off-topic] [Tip] 💡 @JehandadKhan click on the source code link you've provided,

https://github.com/ROCmSoftwarePlatform/MIOpen/blob/e65e04ac1df4fa1381cf6ea439ad22b29998df42/src/include/miopen/conv/problem_description.hpp#L285

then scroll to the top of the page and click [Blame] to see where this line of code came from.

[DrizztDoUrden]

We should update all solvers to the new struct and get rid of miopen::ProblemDescription. That would eliminate unrequited copies of tensor descriptors data an all the mess around it.

[JehandadKhan]

We should update all solvers to the new struct and get rid of miopen::ProblemDescription. That would eliminate unrequited copies of tensor descriptors data an all the mess around it.

I will update the perf db infra to use the new struct.

[atamazov]

@DrizztDoUrden It is so that all new solvers should use miopen::conv::ProblemDescription?

[DrizztDoUrden]

My idea was to smooth transition by making them have 2 GetSolution(), one with old struct and one with ExectutionContext and conv::ProblemDescription. Old API method just transforming params. And later just remove all old methods.

[DrizztDoUrden]

@aserio Do you have in mind any specific topic you expect comments on?

[aserio]

@DrizztDoUrden, I was just adding the label to clarify the purpose of the ticket :)

[atamazov]

@aserio Basically there is nothing to discuss. This is a code quality issue that will be gradually resolved.

[DrizztDoUrden]

To clarify the intent. My idea was to show how to do it in GEMM solver PRs, as I would have to make solvers there. But it's got postponed.

[JehandadKhan]

@DrizztDoUrden we should add all the members of the miopen::ProblemDescription Visitor to the miopen::conv::ProblemDescription before we make the transition.

[atamazov]

@DrizztDoUrden Is it so that currently we need Serialize and mloBuildConf_Key methods from both ProblemDescriptions?

[DrizztDoUrden]

I don't think there should be any point in time when the methods should be used from both simultaneously.

[aserio]

@DrizztDoUrden, would you be able to provide an update on the status of this ticket?

[DrizztDoUrden]

What do you mean?

[aserio]

@DrizztDoUrden I have two primary concerns as this is a high-priority ticket:

1. What are the steps to resolve this issue? (Especially what the the remaining steps to resolve the issue.)
2. What is the current timeline for completion?

[DrizztDoUrden]

Steps to resolve as I see them:

1. Move every solver to the conv::ProblemDescription.
2. Remove the miopen::ConvolutionContext.

Timeline: I am not sure, kind of every other ticket I get lately is a high priority. But I'd like to get into this after Run and Measure elimination. It shouldn't require a lot of time though and the first step may be paralleled.
This is kind of blocker for a sane way of implementing gemm invokers btw, as gemm functions have tensor descriptors as parameters If I remember that correctly.

[aserio]

@DrizztDoUrden have you been able to return to this issue?