Repository navigation
build(flydsl): upgrade FlyDSL to 0.4.0 - #817
Conversation
…htly CI pinned flydsl<0.3 because the cco FlyDSL examples imported flydsl.expr.buffer_ops, which 0.3.0 removed. aiter now pins 0.4.0, so mori was only ever tested against a FlyDSL its users no longer get. - mori.cco.device.flydsl.buffer: create_buffer_resource_from_addr / buffer_load / buffer_store and the CM_* cache policies, moved unchanged from ops/gemm_ar/_compat.py (which re-exports them). It is the shim that keeps cache_modifier working on 0.3.0+; EPv2's flydsl_compat ignores it there. - cco examples 04/05/07/08 use it instead of flydsl.expr.buffer_ops. - Dockerfile.dev: FLYDSL_VERSION build arg, default 0.4.0 (aiter's pin). - nightly.yml: flydsl-compat job builds the image with 0.2.4 and 0.3.2 and runs the FlyDSL-backed tests and examples, so the older paths stay covered. - READMEs: install flydsl==0.4.0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…htly - nightly flydsl-compat: example 07 exits at once without MORI_ENABLE_SDMA, so pass it; test 0.3.0 instead of 0.3.2 -- 0.3.0 is the positional-aux path, 0.3.1+ the keyword one CI's 0.4.0 already covers. - ci_cco: run example 07 too, so the migrated example runs on 0.4.0. - Comments: aux became keyword-only in 0.3.1; T.<dtype> is a property in 0.2.x already; _compat.py no longer claims CI pins flydsl<0.3. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kawhil-amd
left a comment
There was a problem hiding this comment.
Thanks for the shared buffer.py; having the cache-policy encoding in one place is a good change. The first comment is the one I would resolve before merging: the migrated examples appear to pass cache-modifier values from the old scheme into the new helper. The rest are smaller points on error handling, tests, coupling and CI. These come from reading the diff. I did not run them on hardware.
One point has no line in this diff because the file is unchanged: ops/dispatch_combine_v2/flydsl_compat.py is a second FlyDSL 0.3+ buffer shim. It ignores cache_modifier on 0.3+ and probes with ModuleNotFoundError, while buffer.py uses ImportError. The two shims will drift apart. Please reuse buffer.py there, or note why EPv2 must stay separate.
| return buffer_ops.buffer_load( | ||
| rsrc, 0, vec_width=1, dtype=T.i32, cache_modifier=_CM_SC1 | ||
| ) | ||
| return buffer.buffer_load(rsrc, 0, vec_width=1, dtype=T.i32, cache_modifier=_CM_SC1) |
There was a problem hiding this comment.
_CM_SC1 is still the old private constant (_CM_SC1 = 2 at line 68, _CM_SC0_SC1 = 3 at line 69), but this call now goes through buffer.buffer_load. On FlyDSL 0.3+, buffer.py passes aux straight through as the LLVM CPol operand. By the table in buffer.py, 2 is CM_NT there, not "bypass L2" (CM_SC1 = 16). The store at line 91 has the same issue: 3 is sc0|nt, not sc0|sc1 (CM_SC0_SC1 = 17).
If that is right, a spin on a slot that a peer writes can keep reading a stale L2 line and hang, and a signal store may not reach the peer. A 2-rank CI pass does not show that the policy is right. Please confirm which scheme buffer_ops uses on 0.2.x and which one the raw ops use on 0.3+, and use buffer.CM_SC1 and buffer.CM_SC0_SC1 here. If the 0.2.x buffer_ops uses a different numbering, buffer.py also needs a per-version mapping, because it passes CM_* to buffer_ops unchanged there.
There was a problem hiding this comment.
You're right, and it is older than this PR. 0.2.x buffer_ops passes cache_modifier to RawPtrBufferLoadOp/StoreOp as aux unchanged (aux_flags = _create_i32_constant(cache_modifier)), so both paths use LLVM's CPol numbering and no per-version mapping is needed. That also means _CM_SC1 = 2 and _CM_SC0_SC1 = 3 were nt and sc0 nt on every version, main included.
Example 05 now uses buffer.CM_SC0_SC1 for the signal store, buffer.CM_SC1 for the spin load and buffer.CM_CACHED for the output. The new tests/python/cco/test_flydsl_buffer.py compiles a load and a store for each CM_* value and reads the bits back from the ISA. On gfx950, 0.4.0, 0.3.0 and 0.2.4 all give the same result, for both load and store:
CM_* |
0 | 1 | 2 | 16 | 17 |
|---|---|---|---|---|---|
| bits | none | sc0 |
nt |
sc1 |
sc0 sc1 |
| v = buffer_ops.buffer_load(src_rsrc, off, vec_width=4, dtype=T.i32) | ||
| buffer_ops.buffer_store(v, dst_rsrc, off, cache_modifier=_CM_UNCACHED) | ||
| v = buffer.buffer_load(src_rsrc, off, vec_width=4, dtype=T.i32) | ||
| buffer.buffer_store(v, dst_rsrc, off, cache_modifier=_CM_UNCACHED) |
There was a problem hiding this comment.
Same issue as in example 05: _CM_UNCACHED = 3 is a private constant. The shared module defines CM_SC0_SC1 = 17. Please use the shared constant.
There was a problem hiding this comment.
Done: the private constant is gone, and the store uses buffer.CM_SC0_SC1.
| """Buffer load/store with a working cache policy, on FlyDSL 0.2.x through 0.4.x. | ||
|
|
||
| flydsl 0.3.0 removed ``flydsl.expr.buffer_ops``. ``mori.ops.dispatch_combine_v2 | ||
| .flydsl_compat`` bridges that for EPv2, but there ``cache_modifier`` is |
There was a problem hiding this comment.
flydsl_compat is not part of this PR, but it is the second shim for the same problem. See the review summary: it should reuse this module or say why it cannot.
There was a problem hiding this comment.
EPv2 keeps its own shim for now, and flydsl_compat's docstring now says why. It also bridges vector and T, and making it honour cache_modifier would change EPv2's codegen on 0.3+ (the scatter combine's stage-3 loads ask for nt). That needs a perf check first, so I'd rather do it as a follow-up than in the version bump. The import probes do agree now: buffer.py also catches ModuleNotFoundError, and only for flydsl.expr.buffer_ops.
| wherever the policy is the point -- a spin on a slot a **peer** writes needs a | ||
| load that bypasses L2, and a signal store must be visible past L1+L2: | ||
|
|
||
| * ``mori.ops.gemm_ar``'s cross-GPU barrier and SDMA signals, and |
There was a problem hiding this comment.
This says gemm_ar's cross-GPU barrier and SDMA signals need the cache policy. gemm_ar/_compat.py and the note at line 67 below say those are atomics and use no aux bits. Please make the two statements agree.
There was a problem hiding this comment.
Fixed. The docstring now says gemm_ar's use is the kernels_sdma loads of data a peer's copy engine delivered (CM_SC1). It also says the barrier and signal slots are atomics in mori.cco.device.flydsl.signals and use no aux bits.
| buffer_load = _bo.buffer_load | ||
| buffer_store = _bo.buffer_store | ||
|
|
||
| except ImportError: # flydsl >= 0.3.0 |
There was a problem hiding this comment.
except ImportError also catches an ImportError raised from inside a working 0.2.x buffer_ops or flydsl.expr.typing import. The code then silently switches to the raw rocdl path. Consider catching only the buffer_ops import, so another missing module fails loudly.
There was a problem hiding this comment.
Done. It catches ModuleNotFoundError and re-raises unless e.name == "flydsl.expr.buffer_ops", so any other missing module fails loudly.
|
|
||
| # FlyDSL 0.3.1 moved `aux` on the raw buffer ops from a positional i32 | ||
| # *value* to a keyword-only *attribute*. Both spellings are in use here -- | ||
| # aiter's venv has 0.3.0, sglang's has 0.3.2 -- so probe once and adapt |
There was a problem hiding this comment.
The version notes are inconsistent. The comment says aux moved in 0.3.1 and that sglang has 0.3.2, and aiter's venv has 0.3.0. Dockerfile.dev says aiter pins 0.4.0. The nightly matrix tests only 0.2.4 and 0.3.0, so the keyword path is covered only by 0.4.0. If the real boundary is 0.3.2, the nightly comment is wrong and the cutoff is not tested. Also, _AUX_IS_KWARG only recognises two spellings of aux. If the signature changes again, the probe may pick the positional branch and pass an i32 value where an attribute is expected. There is no unit test for buffer.py. A small test that checks which aux operand is emitted for each CM_* value would have caught the silent bug that the comment at line 65 describes.
There was a problem hiding this comment.
I checked the wheels' generated rocdl ops. aux is a positional i32 Value on 0.3.0 and a keyword-only IntegerAttr from 0.3.1 on (the same on 0.3.2 and 0.4.0). So the boundary is 0.3.1: nightly's 0.3.0 leg covers the positional path, and CI's 0.4.0 covers the keyword path. I removed the venv remarks from the comment.
Two more changes came out of this:
- The probe is strict now. It accepts only "keyword-only on both ops" or "positional on both", and otherwise raises
ImportErrorwith the FlyDSL version. - It reads the generated ops in
flydsl._mlir.dialects.rocdl, notflydsl.expr.rocdl. Running the strict probe showed that from 0.3.1,flydsl.expr.rocdlwrapsraw_ptr_buffer_loadbut not the store. That wrapper also silently drops anauxthat is neither an int nor an Attribute.
I added the unit test: tests/python/cco/test_flydsl_buffer.py. It runs in ci_cco.yml (0.4.0) and in both nightly flydsl-compat legs (0.2.4 and 0.3.0). It only runs on gfx94x/gfx95x, because sc0/sc1/nt are the CDNA3/4 spellings. As a check, I set CM_SC1 back to the old value 4, and the test fails with CM=4 load: [[]] != ['sc1'].
| from flydsl._mlir import ir | ||
| from flydsl.expr import rocdl as _rocdl | ||
|
|
||
| from mori.cco.device.flydsl.buffer import ( # noqa: F401 (re-exported) |
There was a problem hiding this comment.
_compat now imports a private name (_dtype) from the cco device package. Importing mori.cco.device.flydsl.buffer also runs mori.cco.device.flydsl/__init__ first, which loads the bitcode locator, handles and _bindings. So any failure in the cco device bindings now breaks import mori.ops.gemm_ar, even for callers that only need buffer ops. A shared utility fits better in a neutral location, and _dtype should be public or stay private to buffer.py.
There was a problem hiding this comment.
_dtype is no longer imported; wave_uniform_i64 uses the public i32_type().
On the coupling: since d9ba2c4 the signal helpers live in mori.cco.device.flydsl.signals, so example 10 no longer imports gemm_ar's private module. import mori.ops.gemm_ar itself does not load _compat: the package imports only _shuffle and layout, and the kernels load lazily. The three modules that import _compat (kernels_lsa, kernels_sdma, kernels_fused) already import mori.cco.device.flydsl themselves. So this adds no new import-time dependency for any caller.
| #: 0.3 and the CI image pins `flydsl<0.3` (the cco Python examples still import | ||
| #: `flydsl.expr.buffer_ops`, which 0.3 dropped). `llvm.inttoptr` is identical in | ||
| #: 0.3, and 0.2.x is still supported (nightly.yml's flydsl-compat job). | ||
| #: `llvm.inttoptr` is identical in |
There was a problem hiding this comment.
Nit: this comment is wrapped badly (llvm.inttoptr is identical in sits alone on a short line). __all__/the re-export list also omits CM_NT and CM_SC0, which buffer.py exports.
There was a problem hiding this comment.
Fixed: the comment, which now lives in signals.py, is rewrapped, and CM_SC0/CM_NT are in __all__.
| # The version aiter pins, so CI runs mori against the FlyDSL it ships with. | ||
| # mori itself also runs on 0.2.x and 0.3.x; nightly.yml's flydsl-compat job | ||
| # builds this image with an older one to keep those paths covered. | ||
| ARG FLYDSL_VERSION=0.4.0 |
There was a problem hiding this comment.
Minor: ARG FLYDSL_VERSION is declared before the apt and ROCm layers but is used only by the last RUN. Some builders include declared build args in the cache key of later RUN steps. The nightly matrix builds with different FLYDSL_VERSION values, so each variant may rebuild the ROCm layers. Declaring the ARG just before the flydsl RUN avoids this.
There was a problem hiding this comment.
Done: WITH_FLYDSL and FLYDSL_VERSION are now declared just before the flydsl RUN, with a comment on why.
Example 10 (#798) imported its signal/flag helpers from mori.ops.gemm_ar._compat, a private module of an op built on cco: the same layering problem buffer.py solved for buffer load/store. - New mori.cco.device.flydsl.signals: signal_ptr, signal_store_u32, signal_load_u32, local_load_u32, local_store_u32 and i32_type, moved unchanged from gemm_ar/_compat.py. Named signals, not signal, so it cannot shadow the stdlib module. - gemm_ar/_compat.py re-exports them and keeps what is tuned for gemm_ar (tile-counter atomics, the copy-engine release fence, wave_uniform_i64), so the kernels and benchmark keep their imports. - Example 10 imports from the new module. - ci_cco.yml and the nightly flydsl-compat job also run example 10. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Examples 04/05 used private cache constants (2, 3) from a different numbering. aux is the LLVM CPol operand on every FlyDSL version (0.2.4 buffer_ops passes cache_modifier through unchanged), so 2 is nt and 3 is sc0|nt; llc for gfx950 confirms. They now use buffer.CM_SC1 / CM_SC0_SC1, i.e. sc1 and sc0 sc1. - buffer.py: fall back to the raw rocdl path only when the flydsl.expr.buffer_ops module itself is missing; refuse an aux signature that is neither 0.3.0's positional value nor 0.3.1+'s keyword attribute; correct the version notes and which gemm_ar code needs the policy; document the 2 GiB byte-offset limit. - New test_flydsl_buffer.py compiles a load and a store per CM_* value and checks the cache bits in the emitted ISA; run in ci_cco and in the nightly flydsl-compat job. - gemm_ar/_compat.py no longer imports the private _dtype; its __all__ lists CM_SC0 and CM_NT. - flydsl_compat.py says why EPv2 keeps its own shim for now. - Dockerfile.dev declares the FlyDSL args next to their only RUN, so a new version no longer rebuilds the ROCm layers. - nightly flydsl-compat: one leg at a time, ci_cco's RDMA env, and the per-version image removed at cleanup. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
CI pins
flydsl<0.3because the cco FlyDSL examples importflydsl.expr.buffer_ops, which 0.3.0 removed. aiter now pinsflydsl==0.4.0(requirements.txt, setup.py, pyproject.toml), so mori was only tested against a FlyDSL its users no longer get. This upgrades CI to 0.4.0 and keeps the older versions mori still supports covered nightly.Changes
mori.cco.device.flydsl.buffer(new):create_buffer_resource_from_addr/buffer_load/buffer_storeand theCM_*cache policies, moved fromops/gemm_ar/_compat.py, which re-exports them so the gemm_ar kernels and benchmark keep their imports. This is the shim that keepscache_modifierworking on 0.3.0+; EPv2'sflydsl_compat.pyaccepts and ignores it there, so the examples could not use that one. On 0.2.x it isflydsl.expr.buffer_ops; only that module being missing selects the other path, and any other import error is raised. On 0.3.0+ it calls the generatedrocdlops and probes whetherauxis a positional value (0.3.0) or a keyword attribute (0.3.1+); any other signature fails at import. It uses the generated ops rather thanflydsl.expr.rocdlbecause from 0.3.1 that module wraps the load (only) and drops anauxthat is neither an int nor an Attribute.mori.cco.device.flydsl.signals(new): the u32 signal-slot accessors (signal_ptr,signal_store_u32,signal_load_u32,local_load_u32,local_store_u32,i32_type), moved unchanged fromops/gemm_ar/_compat.pyfor the same reason: example 10 (example(cco): cco-sdma all-gather with start/wait (async mode) in pcp scenario #798) imported them from that private module._compat.pyre-exports them and keeps what is tuned for gemm_ar (the tile-counter atomics, the copy-engine release fence,wave_uniform_i64). Namedsignals, notsignal, so it cannot shadow the stdlib module.bufferinstead offlydsl.expr.buffer_ops, and example 10 imports fromsignalsinstead ofmori.ops.gemm_ar._compat. Examples 04 and 05 now use the sharedCM_*constants. Their private ones were wrong on every version, main included: 2 and 3 arentandsc0 ntin LLVM's CPol numbering, which both paths pass through asaux, so the signal store and the spin load did not bypass L2.tests/python/cco/test_flydsl_buffer.py(new): compiles a buffer load and store for eachCM_*value and checks thesc0/sc1/ntbits in the emitted ISA (gfx94x/gfx95x).flydsl_compat.py: the docstring says why EPv2 keeps its own shim for now. It also bridgesvectorandT, and honouring itscache_modifieron 0.3+ would change the scatter combine's codegen, which needs a perf check.docker/Dockerfile.dev: newFLYDSL_VERSIONbuild arg, default0.4.0, declared next to the flydslRUNso changing it does not rebuild the ROCm layers.ci_cco.yml: also runs examples 07 and 10 (withMORI_ENABLE_SDMA=1), so every FlyDSL example runs on 0.4.0, and the cache-policy test.nightly.yml: newflydsl-compatjob builds the image with 0.2.4 (theflydsl.expr.buffer_opspath) and 0.3.0 (the last release that takesauxpositionally; 0.3.1+ is the keyword form CI's 0.4.0 covers) and runstest_sdma_api.py, the gemm_ar numerics (test_gemm_ar*.py,test_flydsl_ar.py), ops-v2test_dispatch_combine_v2_intranode.py(FlyDSL backend), the cache-policy test and examples 04/05/07/08/10. The legs run one at a time, because each uses all 8 GPUs. They use the runner's RDMA settings fromci_cco.yml, and each removes its image afterwards.flydsl==0.4.0.The library itself needed no change: outside the two shims nothing uses a 0.2-only API.
Testing
8x MI355X, CI image (ROCm 7.14), CI build flags, every FlyDSL-related
ci_cco.ymlstep plus example 07:test_sdma_api.pytest_gemm_ar_bench.pybench_epOn the final branch (main with #798 merged in, the signal accessors moved, the examples on the shared constants), I re-ran examples 04/05/07/08/10 and the cache-policy test on all three versions; example 10 was run as the CI command is written. I also re-ran the gemm_ar numerics on 0.4.0: 114 passed, 1 skipped. All three versions emit the same cache bits, for both load and store:
CM_*sc0ntsc1sc0 sc1With
CM_SC1set back to 4, the old wrong value, the test fails.0.3.2 was also run (examples, SDMA API, gemm_ar, Triton, JIT binding: pass). The new example commands were run as written: without
MORI_ENABLE_SDMAexample 07 exits 1, with it both the ci_cco and nightly forms pass.On main the four examples fail on 0.3.x/0.4.0 with
cannot import name 'buffer_ops'; everything else above already passed there. Performance: gemm_ar is unchanged across versions; the EPv2 FlyDSL backend's combine is 3-6% faster on 0.3.4.1/0.4.0 than on 0.2.4 (bench_ep, M=512..8192), dispatch unchanged.Note:
test_fused_is_stable_across_repeatshung once on 0.3.2 in a full-suite run. It did not reproduce in 4 isolated runs (2 on main, 2 on this branch) or a second full-suite run, so it looks like an intermittent pre-existing issue rather than a version or branch effect -- worth knowing if the nightly job flakes.🤖 Generated with Claude Code