Repository navigation
Conversation
PerformanceConfigConvHipConv serializes its kernel descriptor (family[field=value,...]). #9680 switched it to this string form but kept it on PerfConfigBase, whose field reader splits on commas, so a multi-field descriptor such as direct[tile_size_k=128,tile_size_n=1,...] was truncated to direct[tile_size_k=128 on read. The config then matched no kernel, the perf-config picker skipped every ConvHipConv candidate, and a tuned config saved to the perf db could not be reloaded. Serialize/Deserialize the descriptor verbatim, the way PerfConfigBaseCK handles CK kernel ids, by deriving from PerfConfig directly. Deserialize clears the resolved index so IsValid() re-resolves the new descriptor, and operator== compares descriptors (the identity) rather than the per-build index cache. Add CPU-only tests covering descriptor round-trip and reload through a DbRecord, which the picker-replay tests in #9680 did not exercise. Co-Authored-By: Claude <noreply@anthropic.com>
With the descriptor read fixed, the ConvHipConv perf-config model becomes reachable, and on the regression set it picks direct[128,1,16,16] almost everywhere: 1.2-1.4x slower than the solver default on large shapes (~0.89x OOTB geomean), helping only some small shapes. Remove its section from lgbm_pcfg.bin and its golden vectors until the model is retrained and validated on full descriptors. The picker then abstains for ConvHipConv and the solver falls back to its own default. Data-only change: the other 13 solver sections are byte-identical. Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
averinevg
reviewed
Oct 8, 2026
| static void Visit(Self&& self, F f) | ||
| void Serialize(std::ostream& stream) const override | ||
| { | ||
| stream.write(descriptor.c_str(), static_cast<std::streamsize>(descriptor.length())); |
Contributor
There was a problem hiding this comment.
Suggested change
| stream.write(descriptor.c_str(), static_cast<std::streamsize>(descriptor.length())); | |
| stream << descriptor; |
Contributor
There was a problem hiding this comment.
[SUGGESTION] The test file is conv_hipconv_perf_config.cpp with suite CPU_ConvHipConvPerfConfig_NONE. The convention for perf-config tests is perf_config_<Solver>.cpp with suites named CPU_PerfConfig_<Solver>_NONE.
| for(const auto& desc : Descriptors()) | ||
| { | ||
| // A comma in the stored value is what the field reader split on. ':' and ';' | ||
| // are DbRecord's own id/entry separators, which a value must not contain. |
Contributor
There was a problem hiding this comment.
DbRecord splits the id from the value at the first : (https://github.com/ROCm/rocm-libraries/blob/develop/projects/miopen/src/db_record.cpp#L111)
Contributor
Author
|
This work is covered by #13213. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a serialization bug in
ConvHipConv's performance config and removes the solver from the shipped perf-config bundle until its model is retrained.PerformanceConfigConvHipConvstores a kernel descriptor string, e.g.direct[tile_size_k=128,tile_size_n=1,tile_size_h=16,tile_size_w=16,aligned=false]. #9680 switched the config to this string form but left it onPerfConfigBase, whose field reader splits on commas. So on read the descriptor was truncated todirect[tile_size_k=128. Consequences:The comma format is not a hipconv defect —
describe_config()has always producedfamily[field=value,...](used by perf logs, #11636). #9680 adopted it as the saved form without making the reader handle commas.JIRA ID : ALMIOPEN-2494
Changes
fix: read ConvHipConv perf config descriptor whole—PerformanceConfigConvHipConvderives fromPerfConfigdirectly (notPerfConfigBase) and serializes/deserializes the descriptor verbatim, the same wayPerfConfigBaseCKhandles CK kernel ids.Deserializeclears the resolved index soIsValid()re-resolves the new descriptor;operator==compares descriptors (the identity) rather than the per-build index cache. Adds a CPU-only gtest (conv_hipconv_perf_config.cpp) covering descriptor round-trip and reload through aDbRecord— the path feat(miopen): Cross-architecture AI heuristics for Navis (immediate mode) #9680's picker-replay tests did not exercise.fix: drop ConvHipConv from the perf-config bundle— once the read is fixed the ConvHipConv model becomes reachable, and on the regression set it picksdirect[128,1,16,16]almost everywhere (1.2–1.4x slower than the solver default on large shapes, ~0.89x OOTB geomean). Its section is removed fromlgbm_pcfg.binand its golden vectors until the model is retrained and validated on full descriptors. Data-only change: the other 13 solver sections are byte-identical. The picker then abstains for ConvHipConv and the solver uses its own default.Test plan
CPU_ConvHipConvPerfConfig_NONE.*(descriptor round-trip incl. the comma case; reload throughDbRecord)PerfConfigBasereader truncatesdirect[tile_size_k=128,...]->direct[tile_size_k=128conv_hipconv.cpp(stub +MIOPEN_USE_HIPCONV=1),solver.cpp, and the new test compile cleanGPU_UnitTestConvSolverConvHipConv*(search + solution), and a perf-db tune→reload cycle confirming the stored descriptor is not truncated and reloads without an "obsolete or corrupt" warningNotes