Repository navigation
fix(miopen): resolve the system DB path from the MIOpen module on Windows - #10838
Merged
Merged
Conversation
✅ 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. |
…dows GetLibPath() was implemented only for Linux, so on Windows GetSystemDbPath() fell back to the configure-time MIOPEN_SYSTEM_DB_PATH. That variable is only set when BUILD_DEV is on, so release builds get an empty path and every system database filename is resolved against the current working directory. An installed MIOpen therefore finds its find-db/perf-db only when the process happens to be started from the install directory, and silently falls back to a full search otherwise. Implement GetLibPath() for Windows using the module that owns miopenCreate, mirroring GetOwningModuleDirectory() in src/solver/ck_impl_lib_loader.cpp, and default the system DB path to that directory. DATABASE_INSTALL_DIR is "bin" on Windows, so the databases are installed next to MIOpen.dll. MIOPEN_SYSTEM_DB_PATH keeps precedence, BUILD_DEV keeps reading from the build tree, Linux behaviour is unchanged, and the previous behaviour is retained if the module lookup fails. Register MIOPEN_SYSTEM_DB_PATH in the debug environment registry so tests can set it, and cover the contract in test/gtest/db_paths.cpp: environment variable precedence, the default being absolute rather than working-directory relative, and the Windows default following MIOpen.dll.
jdcampbe
force-pushed
the
users/jascampb/fix-system-db-path-windows
branch
from
August 18, 2026 22:42
c7320d7 to
aa23a74
Compare
cderb
approved these changes
Aug 19, 2026
cderb
left a comment
Contributor
There was a problem hiding this comment.
Low probability buffer error.
otherwise LGTM
GetModuleFileNameW signals "buffer too small" by returning the buffer size, so the retry loop doubled its buffer with no upper bound. A misbehaving or hooked API that always reports the buffer as too small would grow it until the allocation throws. Windows paths top out at 32767 wide characters, so cap the buffer there, warn, and fall back to the existing empty-path behaviour instead. Applied to both copies of the pattern: GetLibPath() in db_path.cpp.in and GetOwningModuleDirectory() in solver/ck_impl_lib_loader.cpp. Uses a ternary rather than std::min because both translation units include windows.h without NOMINMAX, whose min() macro breaks the std:: call.
jdcampbe
marked this pull request as ready for review
August 21, 2026 17:52
jdcampbe
enabled auto-merge (squash)
August 24, 2026 22:26
…-system-db-path-windows # Conflicts: # projects/miopen/test/gtest/db_paths.cpp
JonathanLichtnerAMD
added a commit
that referenced
this pull request
Sep 2, 2026
## Summary Backfills the **MIOpen 3.6.1 / ROCm 10.1.0** changelog section with externally-visible changes that landed since the `release/therock-10.0` branch but weren't yet documented. All 43 MIOpen commits in that range were reviewed; CI/build-infra, test-only, docs, version-bump, and already-documented changes were excluded. 21 entries added: ### Added * [RNN] Fused single-layer LSTM forward-inference kernel, gated behind `MIOPEN_DEBUG_RNN_FUSED_INFERENCE` (#8915) * [Conv] Naive reference kernel enabling 3D INT8 forward convolution (#10051) * [Conv] `ConvHipConv` solver backed by vendored hipconv kernels (#9746) * [Conv] float8 vector type support + deduplicated math kernels (#10130) * [Conv] Validated gfx1151 and gfx1200 (Navi) SystemDBs (#10050) * [Conv] `MIOPEN_DEBUG_DISABLE_SYSTEM_DB` / `MIOPEN_DEBUG_DISABLE_USER_DB` env vars (#10728) * [Conv] CK depthwise convolution on RDNA wave32 GPUs (#9373) ### Changed * [Conv] Winograd Rage RxS f2x3 on gfx950 + gfx942 v4_6_1/v4_9_1 kernels (#9014) * [Conv] Updated 3D AI heuristics for gfx942 & gfx950 (#8954) ### Optimized * [Conv] Single GEMM for point (1x1) output convolution, all directions, 2D/3D (#10912) * [Conv] Naive backward-data kernel work group widened to 1024 (#10684) ### Resolved Issues * [Conv] Batch-split miscalculation on large WrW tensors — ROCM-26011 (#11188) * [Conv] Conv failure on XNACK-unsupported gfx9 APUs (gfx90c) (#11211) * [BatchNorm] Device heap-overflow in fwd spatial `FinalMeanVariance` — ALMIOPEN-2424 (#11093) * [Conv] Deterministic `SetNextValue` polarity for grouped Bwd/Wrw CK solvers (#10240) * [LRN][LayerNorm] Grid alignment + OOB threads on gfx1250 (#10251) * [Conv] Stale gfx950 grouped-wrw SystemDB entries (#11130) * [Conv][Windows] System-DB path resolution from the MIOpen module (#10838) * [Conv] System-DB fallback so MI308X (gfx942) resolves a usable DB (#8802) * JSON perf logs (`MIOPEN_PERFORMANCE_LOGS`) dropping the last solver during Find (#11166) * Build failure against GCC 15 libstdc++ under C++20 (#9642) ## Notes Deliberately excluded: internal/default-off changes (#10219, #10088, #10649, #10231), a near-duplicate of an existing 10.0.0 entry (#10188), and CI/build-infra/test-only/docs commits. Docs-only change; no code impact. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
On Windows,
miopen::GetSystemDbPath()returns an empty path for anyBUILD_DEV=Offbuild, soevery packaged find-db/perf-db filename resolves against the process's current working directory.
An installed MIOpen therefore loads its system databases only when the application happens to be
started from the install directory, and silently falls back to a full solver search otherwise.
This gives Windows the same install-relative resolution Linux already has, by locating the
directory of the loaded
MIOpen.dll.JIRA ID : ALMIOPEN-1195
Risk Assessment
Risk 3. The change is Windows-only, host-side path resolution — no kernel, dispatch, or
support-surface change. Its blast radius is nonetheless every Windows user, because the intended
effect is that shipped find/perf databases now actually load, which changes solver selection from
a search result to the tuned database entry. Those entries come from the validated databases MIOpen
already ships, so this is the designed behaviour rather than a new heuristic.
MIOPEN_SYSTEM_DB_PATHkeeps precedence,
BUILD_DEVstill reads from the build tree, Linux is untouched, and if the modulelookup ever fails the previous behaviour is retained.
One deliberate behaviour change worth reviewer attention: anything relying on the accidental
working-directory lookup (placing databases beside an executable and launching from there) must now
set
MIOPEN_SYSTEM_DB_PATH=.. The existing comment shows working-directory resolution was afallback, not a design goal, but it is user-visible.
ASIC Coverage
Architecture-independent: the diff touches only host-side path resolution and an environment
registry entry, with no kernel, dispatch, default-selection, or support-surface change. Standard PR
CI is sufficient; no multi-arch sweep is warranted.
Verified end-to-end on Windows on a gfx1151 GPU, against the gfx1151 system databases added by
#10050. The lane that exercises this for real is the Windows gfx1151
db_synctest(
Smoke/CPU_DBSync_NONE.StaticFDBSync), which only has gfx1151 databases to find once #10050 lands.Testing Summary
db_pathsGTest covering environment-variable precedence,the default being absolute rather than working-directory relative, and the Windows default
following
MIOpen.dll.without it.
from an unrelated working directory with no MIOpen environment variables set and an isolated user
database, confirming the solution MIOpen selects is the one recorded in the system database rather
than the result of a fresh search.
pre-commit(clang-format, MIOpen GTest naming) over all changed files.bounded loop (MSVC 14.44 STL, ROCm wheel clang,
BUILD_TESTING=ON,MIOPEN_USE_COMPOSABLEKERNEL=ON), re-ran thedb_pathsGTest, and re-ran the driver from anunrelated working directory to confirm both copies of the loop still resolve the module
directory at runtime. Loop termination itself was additionally exercised standalone against
stubs, since a misbehaving
GetModuleFileNameWcannot be produced in-process.Testing Checklist
build/bin/test_db_paths.exe- Status: Passed (15/15; 3 new)SystemDbPath_NoEnvVar_IsAbsolutefails without the fix withpath '' must be absolute;SystemDbPath_NoEnvVar_FollowsMIOpenModulefails with an empty path instead of the module directory)MIOpenDriver conv|convfp16|convbfp16 ... -V 1over 18 configurations taken from the shipped find-db (fp32/fp16/bf16 x forward/backward-data/backward-weights) - Status: Passed (18/18 located the database and used its recorded solution; 0/18 before the fix, which reportedDatabase directory does not exist)pre-commit run --files projects/miopen/src/db_path.cpp.in projects/miopen/src/env_debug.cpp projects/miopen/test/gtest/db_paths.cpp- Status: Passedninja MIOpen test_db_paths MIOpenDriver(Windows, gfx1151, CK on) - Status: Passed (bothdb_path.cpp.objandsolver/ck_impl_lib_loader.cpp.objrecompiled, no compiler warnings)build-dbtest/bin/test_db_paths.exe- Status: Passed (15/15, including the three new cases)MIOPEN_LOG_LEVEL=6 MIOpenDriver convfp16 -n 4 -c 32 -H 28 -W 28 -k 64 -y 3 -x 3 -p 1 -q 1 -g 4 -I NHWC -f NHWC -O NHWC -F 1 -V 1 -i 1, launched from an unrelated directory - Status: Passed ([GetLibPath] Lib Path: "...\build-dbtest\bin",Loading Composable Kernel dynamic library: ...\build-dbtest\bin\MIOpenCKGroupedConv_gfx1151.dll,Forward Convolution Verifies OK)MAX_PATH+10stub grows once and resolves)pre-commit run --files projects/miopen/src/db_path.cpp.in projects/miopen/src/solver/ck_impl_lib_loader.cpp- Status: PassedSmoke/CPU_DBSync_NONE.StaticFDBSync- exercises the shipped databases once feat(miopen): add validated gfx1151 and gfx1200 Navi system DBs #10050 provides gfx1151 databases - Status: PendingTechnical Changes
src/db_path.cpp.in: implementGetLibPath()for Windows viaGetModuleHandleExW/GetModuleFileNameWon the module owningmiopenCreate, mirroringGetOwningModuleDirectory()in
src/solver/ck_impl_lib_loader.cpprather than introducing a second mechanism.src/db_path.cpp.in:GetSystemDbPath()now returns that module directory on Windows fornon-
BUILD_DEVbuilds.DATABASE_INSTALL_DIRisbinon Windows (CMakeLists.txt:555-557), sothe databases are installed next to
MIOpen.dll. Restructured to an early return so theenvironment override,
BUILD_DEV, Windows, and Linux branches read independently.src/env_debug.cpp: registerMIOPEN_SYSTEM_DB_PATHin the debug environment registry, matchingthe existing
MIOPEN_USER_DB_PATH/MIOPEN_CUSTOM_CACHE_DIRentries, so tests can set it.test/gtest/db_paths.cpp: three cases added to the existingCPU_DbPaths_NONEfixture. TheWindows case resolves the module directory independently inside the test, so it validates the
contract rather than re-running the implementation, and skips under
MIOPEN_BUILD_DEV.src/db_path.cpp.in,src/solver/ck_impl_lib_loader.cpp(review follow-up): cap theGetModuleFileNameWretry buffer at 32768 wide characters — the Windows path ceiling — insteadof doubling without bound, warn, and fall back to the existing empty-path behaviour. Applied to
both copies of the pattern, per review. A ternary is used rather than
std::minbecause bothtranslation units include
windows.hwithoutNOMINMAX, whosemin()macro breaks thestd::call.
Related
closed as completed by ci: point MIOPEN_SYSTEM_DB_PATH at the artifact bin dir on Windows #10081, which only exports
MIOPEN_SYSTEM_DB_PATHin the CI workflow anddoes not change library behaviour for installed users. Its "Impact beyond CI" section asked for
confirmation against a real Windows install; the end-to-end testing above is that confirmation.
Suggest reopening it, or tracking the user-facing half here.
on Windows, and the source of the databases used for verification here.