Open averinevg opened 1 year ago
@atamazov @DrizztDoUrden Please take a look
@averinevg How would this impact the solver registry ? Currently, the solver registry does properly support any primitive other than convolution.
@averinevg I would recommend not breaking apart the functionality used by Fin into a separate class of Solvers. As the functionality Fin will need to access should be inherent to the Solver.
I think it is easy to agree that IsTunable is in the same category of informative functions as IsApplicable and IsDynamic. This would also be important for casting from base solver to tunable and non-tunable solvers.
GetAllSolutions is for sure a tuning specific function as it pulls out details of the process (list of all possible solutions from the solver), but this should be callable from all types of solvers to be effective.
GetPerfCfgParams and TestSysDbRecord (now renamed to TestPerfCfgParams) are vital functions for interacting with the performance database using the serialized PerformanceConfig. These were implemented in any_solver because there wasn't a public access to the PerformanceConfig used by the solver previously. In your proposal it seems this structure would now be retrievable. As a matter of convenience they would be nice to keep, but it appears that the physical limitation from flushing these out in Fin would be lifted.
@cderb Your thoughts about IsTunable and GetAllSolutions look reasonable to me.
Regarding TestSysDbRecord/TestPerfCfgParams -- this is not a Solver's job to access databases. If fin needs to read database and validate a record, then it is fin who should read database, but then fin is free to use Solver API to validate the record.
@JehandadKhan
@averinevg How would this impact the solver registry ? Currently, the solver registry does properly support any primitive other than convolution.
I think that the registry will continue to support all primitives after the proposed changes are made. @averinevg Please confirm.
@JehandadKhan
@averinevg How would this impact the solver registry ? Currently, the solver registry does properly support any primitive other than convolution.
AnySolver
would be replaced with a pointer to SolverBase
inside the solver registry. All functionality will be kept as @atamazov wrote. After that, we could add the necessary functionality for the rest of the primitives.
Update: IsTunable()
has been moved to SolverBase
as proposed by @cderb.
@cderb I believe that any parts of Fin
should not be present in the release build. But we can keep them there for a reasonable transition period. If moving them in Fin
turns out to be a non-trivial task, they will remain inside the base class. What is your opinion?
@averinevg if these two functions can be made accessible from all solvers:
PerformanceConfig GetDefaultPerformanceConfig(const Context&, const Problem& problem);
bool IsValidPerformanceConfig(const Context&, const Problem& problem, const PerformanceConfig& config);
Then I can accomplish the following functions within the fin library:
bool TestSysDbRecord(const ExecutionContext& ctx, const ProblemDescriptionBase& problem, const DbRecord& record);
std::string GetPerfCfgParams(const ExecutionContext& ctx, const ProblemDescriptionBase& problem, Db& db);
It would be helpful if there is some transition time so that I can migrate these functions into fin as well.
Good!
Update: ifdef
TUNA_BUILD
was replaced with MIOPEN_ENABLE_FIN
Update: description has been changed as suggested by @DrizztDoUrden
ProblemDescriptionBase
has been implemented in https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1875
End goals
Steps
ConvolutionContext
andmiopen::ProblemDescription
. It is impossible to generalize contexts without splitting. We would also get the ability to put severalProblemDescriptions
into a vector without being tied toConvolutionContext
. This step is almost done, primitives have been already split inside the solvers. The rest of the work relates to splitting in other parts of the library.miopen::ProblemDescription
with newmiopen::conv::ProblemDescription
. Some new solvers have already usedmiopen::conv::ProblemDescription
. This class was created as a more convenient replacement ofmiopen::ProblemDescription
. This step is optional, but it would be convenient to do this as part of this work in order to reduce technical debt. Solvers would be combined in groups in order to reduce the load on the CI and the number of PRs for review. This task is about replacement of the methods and data fields of the old class with corresponding methods and data fields of the new class, it is not very time-consuming.ProblemDescriptions
. This task will not take much time to implement.SolverBase
in such a way that it can be used instead ofAnySolver
.FindSolution()
should be moved fromAnySolver
to this class, and in existing methods, base classes should be used instead ofboost::any
.AnySolver
withSolverBase*
inside the solver registry.AnySolver
according to https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1250#issuecomment-964377131 as proposed by @pfultz2.Implementation
A simplified version of the future base class is shown below:
We have several methods inside
AnySolver
that are only needed forTuna
. My suggestion is to move them to separated class.This is a mixin class which is already implemented
Base class for non tunable solvers is show below. It contains only one method
Base class for tunable solvers is shown below