[tuning] Do not insert default perf-config into empty tuning container

[atamazov]

An addition to #2279, a partial fix for #2270. Resolves #2279 (comment).

---

[Attribution] @junliume @JehandadKhan

• https://github.com/ROCmSoftwarePlatform/MIOpen/labels/quality
• https://github.com/ROCmSoftwarePlatform/MIOpen/labels/urgency_normal
• https://github.com/ROCmSoftwarePlatform/MIOpen/labels/value_high - It is important to not camouflage problem in the tunable Solvers.
• Proposed reviewers:
  • @JehandadKhan
  • @junliume
  • @shurale-nkn
  • @DrizztDoUrden

[shurale-nkn]

NO. This change contradicts to the requirements for the mentioned functions.

GetDefaultPerformanceConfig is only one function in solver which shall return valid result if IsApplicable=true.
https://github.com/ROCmSoftwarePlatform/MIOpen/blob/b8b51dde22f6d3b3b18e01a1b6c3c5caf570470b/src/include/miopen/solver.hpp#L97-L102

https://github.com/ROCmSoftwarePlatform/MIOpen/blob/b8b51dde22f6d3b3b18e01a1b6c3c5caf570470b/src/include/miopen/solver.hpp#L189-L200

Also, the ComputedContainer definition says that configs generated from "spare" and "main" sets cannot cover all possible combinations of available parameters. At the same time result of IsApplicable not limited and based on all possible combinations, so Is Applicable cannot guarantee that the ComputedContainer will not be empty.
https://github.com/ROCmSoftwarePlatform/MIOpen/blob/b8b51dde22f6d3b3b18e01a1b6c3c5caf570470b/src/include/miopen/generic_search.hpp#L135-L149

[DrizztDoUrden]

Technically LGTM, but what Kamil said should be resolved in one way or another and I am not the one to talk about perf params of specific solvers.
I would like to add though it is possible to add the default config to containers of all solvers that need it, but it would require additional work.
The current proposal is functional in nature, meaning that how tuning functions would change. Before and after my PR default config was used as a failure recovery option. The change disables that entirely as solver is considered faulty for the specific config. This should be considered before merging.
A compromise solution is to instead produce some message in case the default config has been added, but I am not sure what severity it should have. That would not change the behaviour, but given the circumstances it would probably be completely ignored by everybody.

[atamazov]

@shurale-nkn

This change contradicts to the requirements for the mentioned functions.

I do not see any contradiction. Requirement reside in ticket #866, esp. pls see "Restrictions" at #866 (comment)

[shurale-nkn]

@shurale-nkn

This change contradicts to the requirements for the mentioned functions.

I do not see any contradiction. Requirement reside in ticket #866, esp. pls see "Restrictions" at #866 (comment)

@atamazov
This section was changed simultaneously with this PR creation. But the majority of our solvers were created and approved long before.

You can change solver requirements and their behavior, but this must be done simultaneously with solvers update or after .
The current change creates a bug in library behavior. And this is very strange to make the authors of solvers responsible for this bug, since there were no such requirements at the time of code creation.

[atamazov]

@shurale-nkn

This section was changed simultaneously with this PR creation.

Please look at the history more closely. I specifically clarified the restrictions 3 weeks ago when I was on vacation, before #2279 was created.

This PR removes code changes made in #2279 that are not needed to resolve #2270.

[atamazov]

@shurale-nkn I periodically update the requirements to make them clearer. Usually, as soon as I see some ambiguities or omissions that can lead to discrepancies. I do this primarily for the convenience of programmers, and not (as one might think) to confuse them. Such changes are always made in accordance with the original intent; otherwise the changes are marked as "QUIET CHANGE". I think you do that same when you write specs.

[shurale-nkn]

My comments are not about confused programmers.
We have solvers, and they were developed in different time periods and in accordance with the requirements of that time.
We have a search engine that interacts with solvers within these requirements.
And now we are changing these requirements without changing the solvers. This creates a situation in which the solver, which was considered as properly functioning yesterday, becomes faulty due to bug. And now we consider as "bug" not changes in interaction with solvers, but their internal functionality. ( the situation when IsApplicable()=true, but 'GetDefaultPerformanceConfig() -> failed' does not apply to this example, this is bug in any version of requirements. )

The current version of the ComputedContainer cannot guarantee that the container is not empty when IsApplicable()=true, because from information in code (and I prefer to think that the function description in the code has a higher priority, because it needs to be approved and merged) IsApplicable and GetDefaultPerformanceConfig operates over full list of solver parameters and their combinations, and should have a direct relationship. But GenericSearch uses only limited set of combinations based on "spare" and "main" set of perf configs, which means that there is a possibility that a single working combination is located outside the coverage of these sets, since their coverage is not identical to the coverage of IsApplicable, and in this case only GetDefaultPerformanceConfig can and should return the valid combination .

For this PR, need to manually check all solvers for the presence of GetDefaultPerformanceConfig in the code of the interface working with GenericSearch and if this is need to add it, because there was no such requirement before, according to the description of the ComputedContainer and its fields.

[atamazov]

Converted to draft as I can't convince Kamil. If he doesn't change his mind within a week, this PR should be closed and the necessary changes should be made in the Solver/Invoker spec (#866).

[atamazov]

#2296 (comment)