Skip to content

Fix: dispatch device memory wrappers by exact source/destination device pair - #7941

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/device-memory-dispatch-exact-pairs
Sep 9, 2026
Merged

Fix: dispatch device memory wrappers by exact source/destination device pair#7941
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/device-memory-dispatch-exact-pairs

Conversation

@Critsium-xy

Copy link
Copy Markdown
Collaborator

Reminder

  • I have read AGENTS.md and docs/developers_guide/agent_governance.md.
  • I have linked an issue or explained why this PR does not need one.
  • I have added adequate unit tests and/or case tests, or explained why not.
  • I have listed the exact verification commands run and their results.
  • I have described user-visible behavior changes, including INPUT parameter changes.
  • I have explained core-module impact for ESolver, HSolver, ElecState, Hamilt, Operator, Psi, or other source/ changes.
  • I have requested any needed governance exception below.

Linked Issue

Fix #7553

Unit Tests and/or Case Tests for my changes

  • Commands run (Linux, GCC, OpenMPI, cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON):
    • cmake --build build -j15 -- -k 0 — full build, 3318/3318 targets, 0 errors.
    • ctest --test-dir build -R MODULE_BASE_DEVICE -V — passed, including the three new CPU dispatch tests.
    • ctest --test-dir build -R 'MODULE_PW_pw_test$|MODULE_ESTATE_elecstate_pw|MODULE_BASE_DEVICE' — 3/3 passed. These two targets compile memory_op.cpp directly in a reduced configuration, so they confirm the new tool_quit.h dependency and the explicit instantiations link there as well.
  • Checks not run, with reason: the four-pair GPU tests are guarded by __UT_USE_CUDA/__UT_USE_ROCM and were not executed — no GPU on the machine used. They are the ones that exercise the actual host/device mis-dispatch and need a GPU validation run.

What's changed?

synchronize_memory() and cast_memory() pick a compile-time specialization from a pair of runtime AbacusDevice_t values, but joined their branch conditions with ||:

if (device_type_out == CpuDevice || device_type_in == CpuDevice) {
    synchronize_memory_op<FPTYPE, DEVICE_CPU, DEVICE_CPU>()(arr_out, arr_in, size);
}
else if (device_type_out == CpuDevice || device_type_in == GpuDevice) { ... }

The first branch matches whenever either side is the CPU, so (GPU, CPU) and (CPU, GPU) both reach the CPU-to-CPU specialization — a plain host memcpy/cast on a device pointer. (GPU, GPU) matches the second branch for the same reason. The four combinations are mutually exclusive, so each branch has to test both devices with &&. That is the fix.

Two things fall out of it:

  • The GPU branches are now inside #if __CUDA || __UT_USE_CUDA || __ROCM || __UT_USE_ROCM, matching the guard on the *_op GPU specializations in memory_op.h. Without it these wrappers cannot be instantiated at all in a CPU-only build, because synchronize_memory_op<T, DEVICE_CPU, DEVICE_GPU> then resolves to the primary template, whose operator() has no definition.
  • An unsupported device combination (e.g. DspDevice, UnKnown) now hits WARNING_QUIT instead of falling out of the if/else chain and silently leaving the destination buffer untouched.

Severity, stated plainly: the defect is currently latent. Both wrappers are declared in memory_op.h but were defined in the .cpp with no explicit instantiation, so they cannot be linked from another translation unit, and grep finds no call site anywhere in source/. Nothing miscopies today. What this PR fixes is a trap: the first caller to use the documented API would have got a silent host memcpy on a device pointer, with no diagnostic.

So the PR also makes the declared API real — explicit instantiations for synchronize_memory (int, float, double, std::complex<float>, std::complex<double>) and for cast_memory (only the eight type pairs that cast_memory_op provides for all four device combinations, so the instantiation list is identical in CPU-only and GPU builds) — and adds unit tests in source_base/module_device/test/memory_test.cpp that pin the dispatch: CPU-to-CPU in every configuration, and all four pairs under __UT_USE_CUDA/__UT_USE_ROCM.

Not changed, and worth a separate look: resize_memory(), set_memory() and delete_memory() dispatch on a single device, so they have no || defect, but they too fall through silently on an unrecognised AbacusDevice_t. I left them alone to keep this diff scoped to the issue.

Governance Notes

  • INPUT/docs changes: none. Internal device-dispatch fix; no INPUT parameter, no output format, no user-visible behaviour change, so docs/parameters.yaml and input-main.md are untouched.
  • Core module impact: none of ESolver / HSolver / ElecState / Hamilt / Operator / Psi are touched. The change is confined to source_base/module_device. memory_op.cpp gains an #include "source_base/tool_quit.h", which is not a new module-level dependency — device.cpp in the same directory already uses ModuleBase::WARNING_QUIT.
  • No new GlobalV / GlobalC / PARAM references; no new default arguments; no new source files.
  • Exceptions requested: none. The governance check reports only the standard "no docs change" warning, addressed above.

🤖 Generated with Claude Code

`synchronize_memory()` and `cast_memory()` select a compile-time
specialization from a pair of runtime `AbacusDevice_t` values, but joined
their branch conditions with `||`. The first branch therefore matched
whenever *either* side was the CPU, so a CPU->GPU or GPU->CPU transfer was
served by the CPU-to-CPU specialization, i.e. a plain host `memcpy`/cast on
a device pointer. GPU->GPU could likewise match an earlier mixed-device
branch. The four combinations are mutually exclusive and must be tested
with `&&`.

The GPU branches are now compiled only when a GPU backend is enabled, which
matches the guard on the `*_op` GPU specializations in memory_op.h, and an
unsupported device combination fails loudly instead of falling through
silently.

Both wrappers are declared in memory_op.h but were defined in the .cpp with
no explicit instantiation, so they could not be linked from another
translation unit and had no call sites -- which is why the defect was
latent. Add the instantiations (for `cast_memory`, only the type pairs that
`cast_memory_op` provides for all four device combinations) and unit tests
that pin the dispatch: CPU-to-CPU everywhere, and all four pairs under
__UT_USE_CUDA/__UT_USE_ROCM.

No documentation change: this is an internal dispatch fix with no INPUT,
output or user-visible behaviour change.

Fixes deepmodeling#7553

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Critsium-xy

Copy link
Copy Markdown
Collaborator Author

Note for reviewers, since attempt 1 of Test on CUDA Build shows as failed in the history.

That run failed on 097_PW_PBE0_FM, which is unrelated to this PR:

[WARNING] etotref cal=-6382.45991959 ref=-6382.46095845 deviation=-0.00103886

The case's threshold is 0.001, so it missed by 3.9%. It cannot be caused by this diff — the two functions changed here have no call sites anywhere in source/, so nothing in a PBE0 GPU run reaches them — and the sibling PR #7942, which contains no source change at all, passed the same job in the same window.

Rather than argue that, I reran the job: attempt 2 passed (1h8m). So this is a marginally-tuned GPU hybrid-functional case, in the same family as #7925, and not a regression from this PR. All 17 checks are green now.

Worth someone's attention separately: a case sitting 3.9% from its threshold on GPU will keep producing spurious red PRs.

@mohanchen mohanchen added GPU & DCU & HPC GPU and DCU and HPC related any issues Refactor Refactor ABACUS codes labels Sep 9, 2026
@mohanchen
mohanchen merged commit 7128060 into deepmodeling:develop Sep 9, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GPU & DCU & HPC GPU and DCU and HPC related any issues Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Dispatch device memory wrappers by exact source and destination devices

2 participants