Repository navigation
build(cco): compile the SDMA path by default - #710
Merged
Merged
Conversation
`BUILD_CCO_SDMA` was OFF unless `BUILD_BENCHMARK=ON` turned it on. What OFF produces is a build whose SDMA puts compile away: every symbol is still there, every kernel still launches, every put still returns, and no bytes move. The all-reduce then returns mostly the local slice. `GemmAllReduceOp.self_test` exists because of one such run -- it measured *faster* than the real thing (-6.8% against -2.3%) precisely because it was not moving data, and the only signal that caught it was perplexity, 862511 against 3.26. Measured, same tree, clean build each side: OFF wall 27s libmori_cco.so 1,701,544 B ON wall 26s libmori_cco.so 1,707,632 B (+6,088 B, +0.36%) It adds no dependency: `anvil.cpp` is in MORI_APP_SOURCES unconditionally and `find_package(hsakmt REQUIRED)` is too, so the host transport and its link deps were already there either way. The flag reaches only cco's own layer -- the queue setup in `cco_init.cpp`, the `cco_sdma_*` wrapper symbols (58 lines of 316), the device API in `cco.hpp`, and one extra JIT bitcode variant that is cached per config. And it creates nothing on its own. `ccoSdmaSetupCommQueues` returns before any allocation unless `MORI_ENABLE_SDMA` is set, which is an independent runtime gate that stays off by default -- verified by building ON and creating a two-rank communicator with and without the variable: both succeed, and the one without allocates no queues. So for anyone not opting in, this changes nothing; for anyone who does, it is the difference between working and silently inert. CI already built with `BUILD_CCO_SDMA=ON` in both `ci_cco.yml` and `ci.yml`, so ON is the configuration that was being tested and OFF was the less-covered one. Also add the diagnostic that was missing on the other side of the gate: when a caller explicitly asks for queues (`sdmaQueueCount > 0`) and none will be created, say which of the two reasons applies. It stays silent for the default `0`, so a comm that never wanted SDMA logs nothing. Not fatal -- a comm without SDMA is legal and every other path still works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seven fixes from reviewing the first commit. Two of them mattered. **The diagnostic was silent in the one build that fails silently.** It sat inside `#if BUILD_CCO_SDMA`, so a BUILD_CCO_SDMA=OFF build -- the escape hatch this PR keeps, and what every older image ships -- still said nothing while the identical configuration on an ON build was diagnosed. That is backwards: OFF is the build whose puts move no bytes. Moved out of the guard, with its own reason string. **`BUILD_CCO_SDMA=1` built a host library with SDMA and a device wrapper without it.** setup.py passed the raw value to CMake, which takes 1/TRUE/yes as true, but baked the flag with `== "ON"`, which does not. So `BUILD_CCO_SDMA=1 pip install .` produced host queues and device puts that compile away -- the same silent-zeros failure this default exists to prevent, entered from the other side. Normalised once, before both uses. Verified: `BUILD_CCO_SDMA=1` now bakes True. The rest: - Widen the gate. `sdmaQueueCount` defaults to 0 and almost every caller leaves it there, so gating on `> 0` kept the common configuration silent. MORI_ENABLE_SDMA is also a way of asking. - Guard `getSdmaQueue`, which returns null for a pair it never connected or a channel past what that pair got, and was dereferenced unchecked. Pre-existing, but this PR is what compiles that loop into every default build. - Keep one CI job on BUILD_CCO_SDMA=OFF. It used to be covered for free by being the default; without it the `#else`, the cco.hpp fallback and the cpp-test skip rule break only for whoever sets the escape hatch. - sweep.py read a missing `_build_flags` as OFF and aborted. With ON the default that guess is wrong; None means unknown, as op.py already had it. - "MORI_ENABLE_SDMA is not enabled", since IsEnvVarEnabled is also false for =0. - Trim the rationale comment and fix the docstring in test_gemm_ar_op.py that still called OFF the default. Two findings checked and not taken. `anySdmaCapable` scans worldSize while the fill loop covers only the lsa range, but `canSDMA == sameHost` and the fill includes self, so the all-null table it implies cannot happen. And cco.hpp's `#define BUILD_CCO_SDMA 0` fallback is left alone: flipping it would make an out-of-tree TU emit SDMA code against a library that may be OFF, which is the same mismatch pointing the other way. Verified on MI355X, both builds, three configurations each: silent when nobody asked, diagnosed when asked and unavailable, working when enabled. Co-Authored-By: Claude Opus 5 (1M context) <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.
BUILD_CCO_SDMAwas OFF unlessBUILD_BENCHMARK=ONturned it on. What OFF produces is a build whose SDMA puts compile away: every symbol is still there, every kernel still launches, every put still returns, and no bytes move. The all-reduce then returns mostly the local slice.GemmAllReduceOp.self_testexists because of exactly one such run. It measured faster than the real thing (-6.8% against -2.3%) precisely because it was not moving data, and the only signal that caught it was perplexity: 862511 against 3.26.What it costs
Same tree, clean build each side:
libmori_cco.soOFFON+6,088 B (+0.36%), no measurable build time.
It adds no dependency.
anvil.cppis inMORI_APP_SOURCESunconditionally andfind_package(hsakmt REQUIRED)is too, so the host transport and its link deps were already there either way. The flag reaches only cco's own layer: the queue setup incco_init.cpp, thecco_sdma_*wrapper symbols (58 lines of 316), the device API incco.hpp, and one extra JIT bitcode variant that is cached per config.Why it is safe
It creates nothing on its own.
ccoSdmaSetupCommQueuesreturns before any allocation unlessMORI_ENABLE_SDMAis set, and that runtime gate is independent and still defaults off.Verified on the new default build, two ranks:
MORI_ENABLE_SDMAsdmaQueueCount1So for anyone not opting in this changes nothing; for anyone who does, it is the difference between working and silently inert.
CI already built with
BUILD_CCO_SDMA=ONin bothci_cco.ymlandci.yml, so ON was already the configuration under test and OFF was the less-covered one.The diagnostic
Case B above used to be silent too. When a caller explicitly asks for queues (
sdmaQueueCount > 0;0means "whatever the env wants") and none will be created,cco_init.cppnow says which of the two reasons applies:At ERROR level rather than WARN because the default global level is ERROR — a warning here would be invisible to exactly the person it is for. Not fatal: a comm without SDMA is legal and every other path still works. Silent for the default
sdmaQueueCount=0, so a comm that never wanted SDMA logs nothing.Verification
On MI355X, from this branch:
pip install .bakesBUILD_CCO_SDMA=True;BUILD_CCO_SDMA=OFFstill builds and bakesFalse— the escape hatch workstests/python/cco/test_sdma_api.py— 12 passedtest_rocm_bootstrap.py+test_rocm_import_order.pywithMORI_ENABLE_SDMAunset — 9 passedOne note on the box used: an unrelated container was holding SDMA engines part of the time, and
sdma_queue_count=8failed withERROR code: 6atanvil.cpp:237while 1 and 2 succeeded. That is the contentionkernels_sdma.pyalready documents ("56 queues to use 7"), and6isHSAKMT_STATUS_NO_MEMORY— a process-memory condition, not queue exhaustion (see #685). It cleared on its own and case C passes above; this change does not touch queue creation.Docs
Updated the two places that called OFF "the default" and the build instructions that told users to pass the flag by hand. The
BUILD_CCO_SDMA=ONrequirement is unchanged everywhere — only the need to type it.🤖 Generated with Claude Code