[BugFix][Transform] Never classify side-effecting binds as replayable (atomics re-executed at every use site since v0.1.11)#2651
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change prevents replay of side-effecting scalar binds and adds regression coverage for atomic return materialization, transformed IR preservation, compaction correctness, and top-k recall. ChangesAtomic bind correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@regression-perf |
Performance Regression Test ReportTriggered by: @LeiWang1999 Results
Artifacts
|
9438612 to
0c380dd
Compare
|
The What happened:
Reproduction (B200, CUDA 13.3, single GPU, A/B interleaved,
The branch has been updated onto current |
IsReplayableScalarBind (introduced in tile-ai#2245) decides whether a scalar bind may be dissolved and its value re-evaluated at every use site (IfStmtBinding, default-on) or replayed across pipeline stages (PipelinePlanning, InjectSoftwarePipeline). The predicate checked the dtype and read/write disjointness, but never the value's effect kind, so a bind like pos = T.atomic_add(counter[0], 1, return_prev=True) used at two sites (bound check + store index) was inlined into both use sites, executing the atomic once per use site instead of exactly once and silently corrupting compaction/histogram/top-k kernels since v0.1.11. The atomic intrinsic is correctly registered kOpaque; its pointer argument enters the expression through tvm_access_ptr, which the access collector records as a read, so the write-disjointness check passed and the bind was declared replayable. Reject any bind whose value has SideEffect > kReadState, mirroring the effect guard StmtSimplifier already applies to let inlining. Read-only values remain replayable; read consistency is enforced by the existing write-set disjointness check. The shipped examples/deepseek_v32/topk_selector.py uses exactly this pattern and is miscompiled on v0.1.11/v0.1.12; its test only printed recall and could not fail, so it also gains a recall assertion. Regression tests: - transform: IfStmtBinding must keep a bind whose value is an atomic RMW returning the previous value. - language (CUDA): compaction kernel using atomic_add(return_prev=True) must contain exactly one AtomicAddRet and produce a permutation with counter == N.
0c380dd to
d0aba87
Compare
|
Rebased onto latest main to resolve conflicts. |
Summary
Since v0.1.11, a scalar bind whose value carries a side effect is dissolved by
tl.IfStmtBindingand re-evaluated ("replayed") at every use site. The canonical victim is the documented usage ofatomic_add(..., return_prev=True):The atomic executes once per use site instead of exactly once — silent data corruption for any compaction / histogram / top-k style kernel that uses the returned value more than once.
v0.1.8–v0.1.10compile the same source correctly; the regression is present inv0.1.11,v0.1.12(PyPI latest) and currentmain.Reproduction
Minimal kernel (added as a regression test in this PR):
v0.1.12 / main lower this to CUDA with the atomic inlined into four sites (the user's capacity guard, the store index, plus the two out-of-bounds guards that
tl.LegalizeSafeMemoryAccesslater builds by copying the already-inlined index expression):Functionally (n = cap = 4096, all-positive input; correct result is
counter == 4096andouta permutation of0..4095):AtomicAddRetin lowered CUDAIn a larger radix-top-k kernel using this idiom (2048 rows × 12288 candidates, top-1024) the corruption is subtle enough to ship: per-row recall vs
torch.topkdrops from 1024/1024 to min 911 / mean ≈1002, the selected set changes across identical reruns (uninitialized shared memory is consumed), and out-of-range garbage indices appear that crash downstream gathers. We hit this in production code derived from thedeepseek_v32/topk_selector.pyexample.Note that the shipped
examples/deepseek_v32/topk_selector.pyitself escapes the bug only accidentally: it routes every atomic return value throughT.alloc_var(a buffer store rather than a scalar bind — required anyway because the eager builder forbids rebinding). Nothing tests or documents that this indirection is load-bearing for correctness; the plain binding shown inatomic_add's own docstring (old_value = atomic_add(counter, 5, return_prev=True)) is the form that miscompiles.Root cause
#2245 introduced
IsReplayableScalarBind()(src/transform/common/bind_utils.h), used to decide that a scalar bind can be dissolved and its value re-evaluated at each use:tl.IfStmtBindinginlines "replayable" binds while pushing theifguard into each statement of the sequence (BindIfStmtWithReplayableBindInlining, configtl.if_stmt_binding_inline_replayable_binds, default true);PipelinePlanning/InjectSoftwarePipelineuse the same predicate to pull "replayable" binds out of pipeline stages and replay them.The predicate checks (a) that the bind is a non-pointer scalar (pointer exclusion added in #2278), and (b) that the value reads no buffer written in the same scope — but it never consults the value's effect kind.
tl.atomic_add_ret_elem_opis correctly registeredkOpaqueinsrc/op/builtin.cc; however its buffer access enters the expression throughtvm_access_ptr, which the access collector records as a read, so check (b) passes and the side-effecting bind is declared replayable.StmtSimplifieralready guards its own let-inlining correctly (SideEffect(op->value) <= CallEffectKind::kPure,src/transform/simplify.cc); the new predicate simply lacks the equivalent guard.Fix
One check in the shared predicate: a bind whose value has
SideEffect(...) > CallEffectKind::kReadState(i.e.kUpdateState/kOpaque) is never replayable. This covers all three consumers (IfStmtBinding,PipelinePlanning,InjectSoftwarePipeline) at once. Read-only values remain replayable — replay-read consistency is exactly what the existing write-set disjointness check enforces.Tests
testing/python/transform/test_tilelang_transform_if_stmt_binding.py::test_if_stmt_binding_keeps_side_effecting_bind— pass-level:IfStmtBindingmust keep a bind whose value is an atomic RMW returning the previous value (structural equality).testing/python/language/test_tilelang_language_atomic.py::test_atomic_add_return_prev_materialized_once— end-to-end CUDA: exactly oneAtomicAddRetin the generated source,counter == N, output is a permutation.examples/deepseek_v32/topk_selector.py—test_topk_selectorcomputed recall but only printed it, so it could never fail on miscompilation; it now assertsrecall >= 0.999(tie-tolerant; the plain-bind miscompile measures ≈0.98). This also protects the example's currently-accidental alloc_var immunity against future refactors.Verification (B200 / sm_100a, CUDA 13.3):
testing/python/transform+testing/python/language: 13 failed / 953 passed / 19 skipped; the 13 failures are pre-existing, not caused by this change (the identical failure set reproduces on an unpatched build of the same base commit on the same machine: tcgen05 codegen x2, TMA gather/scatter x4, cooperative grid_sync, cutedsl PDL, reduce tolerance x3, clear matmul, hoist_broadcast dtype3). No new failures.ruff check/ruff format --check/clang-format --dry-run -Werrorall clean on the changed files.Workarounds for users pinned to v0.1.11/v0.1.12
tilelang<=0.1.10; ortl.if_stmt_binding_inline_replayable_binds=False(covers theIfStmtBindingpath only); orpos = T.alloc_var("int32"); pos[0] = T.atomic_add(..., return_prev=True)(what the deepseek_v32 example happens to do).Fixes #2650
Summary
IsReplayableScalarBind()so scalar binds are not replayable when their value has effects stronger thankReadState. This prevents multiple executions of side-effecting operations (e.g.,T.atomic_add(..., return_prev=True)) that could corrupt compaction and histogram/top-k outputs.AtomicAddRetand checks counter/output correctness.IfStmtBindingto confirm a side-effecting bound value is not rewritten in a way that would replay the atomic at each use.examples/deepseek_v32/topk_selector.pytest by adding a per-row assertion enforcingrecall >= 0.999with improved failure diagnostics for tie/precision issues.C++ style / lint notes
src/transform/common/bind_utils.h(IsReplayableScalarBind()); does not modifydocs/developer_guide/cpp_style.md.C++ API Style Audit (warning only)items (e.g., TLCPP003/TLCPP004) should be treated as advisory unless the PR adds a concrete API/maintainability risk; correctness/build impacts are not expected from style-only findings.