Count asynchronous global copies as unsupported instead of dropping them - #4
Count asynchronous global copies as unsupported instead of dropping them#4SHTUSIST wants to merge 4 commits into
Conversation
`cp.async` and the bulk tensor copy instructions read global memory without going through a register. The rewrite pattern in src/ptxpass_hbf/ptx_memory_op.cpp only matches `ld`/`st` `.global`, and the unsupported pattern in src/ptxpass_hbf/transform.cpp listed `atom`, `red`, non-global `ld`/`st`, `tex`, `suld`, `sust` and inline `asm`. An asynchronous copy matched neither, so an HBF address reached by one produced no entry of any kind: no modeled delay, and no unsupported-list entry either. The coverage record showed no trace the instruction had been there. Line 27 of docs/superpowers/specs/2026-08-09-hbfsim-hybrid-design.md sets the goal this violates: "Fail closed whenever an HBF address could reach an uninstrumented or unsupported memory operation." The two new alternatives require `.global` in the opcode on purpose. The same families carry pure synchronisation forms -- cp.async.commit_group, cp.async.wait_group, cp.async.bulk.wait_group, cp.async.mbarrier.arrive -- which touch no memory, and a shared-to-shared bulk copy names no global space either. None of those may be reported as unsupported memory operations. WHAT THIS CHANGES DOWNSTREAM. The unsupported list is filtered by hbf_relevant_unsupported_opcode in src/ptxpass_hbf/plugin.cpp, written into the module manifest, and consumed at src/cuda_runtime/coverage.cpp:379-396. There are two outcomes, and the second needs a decision: - On a timing-backed range under a non-strict policy, the launch is still allowed and is recorded as `opaque_unmodeled_timing` with `modeled: false`. This is a pure visibility gain: the access was already uncharged, and now it is uncharged and recorded. - Under a strict policy, and on a capacity-backed range, the launch is refused with reason `unsupported_operation`. A kernel that uses these instructions against a registered range and used to run will now be refused. That is what failing closed means here, but whether you want it turned on now is yours to decide, since Triton emits `cp.async` freely on recent architectures. tests/cpu/ptx_async_copy_coverage_test.cpp fails on the previous pattern at the first asynchronous copy and passes on this one. Each case is measured as an increment over the same kernel without the instruction, because the kernel needs a `ld.param` to load its pointer and `ld.param` is itself on the unsupported list; counting absolute totals would have reported every case as unsupported and hidden the defect. No case in the file rewrites an instruction, so the file does not need the embedded device helper and runs in a CPU-only build; the rewriting cases stay in ptx_transform_test. Test suite before and after: the same four tests fail (context_lifecycle, vmem_tuning, run_with_bpftime, mqsim_benchmark), all pre-existing in a CPU-only build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first version of this pattern matched every `cp.async` naming `.global`, including the bulk tensor forms. Branch feature/sm120-exact-stage1 models those forms rather than refusing them: parse_tma in src/ptxpass_hbf/ptx_async_op.cpp accepts exactly three prefixes -- cp.async.bulk.tensor., cp.reduce.async.bulk.tensor. and cp.async.bulk.prefetch.tensor. -- and gives the third of them its own direction, TmaDirection::Prefetch, with a different completion rule. Marking those unsupported here would refuse, once that branch merges, exactly the launches it can model. Two negative lookaheads now exclude the three prefixes. What remains is the gap that branch leaves open: its unsupported pattern is character-for-character the one on hybrid and carries no `cp.` alternative either, so the plain cp.async.ca/cg.shared.global form -- the one Triton emits freely on earlier architectures -- is matched by neither of its patterns. The two lines together now cover the family without overlapping. Test suite: the same four tests fail before and after (context_lifecycle, vmem_tuning, run_with_bpftime, mqsim_benchmark), all pre-existing in a CPU-only build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zzyuanyi
left a comment
There was a problem hiding this comment.
Blocking issue: this PR excludes cp.async.bulk.tensor., cp.reduce.async.bulk.tensor., and cp.async.bulk.prefetch.tensor.* from the unsupported pattern, and its test explicitly requires them not to be counted. The current hybrid base has no ptx_async_op.cpp, parse_tma, or TmaDirection implementation, so those instructions are neither modeled nor reported as unsupported after this PR. That preserves the exact fail-open coverage hole the PR is meant to close. The added ptx_async_copy_coverage and existing ptx_transform tests both pass, but the new negative assertions encode behavior that is only safe after feature/sm120-exact-stage1 is merged. Please either keep these families unsupported on hybrid for now, or make the SM120 modeling change an actual merged dependency before excluding them.
The previous commit excluded cp.async.bulk.tensor., cp.reduce.async.bulk.tensor. and cp.async.bulk.prefetch.tensor. so this pattern would not collide with parse_tma on feature/sm120-exact-stage1. The review is right that this was the wrong trade. hybrid has no parse_tma, no ptx_async_op and no TmaDirection, so on the only branch that exists today the exclusion left those instructions neither modeled nor reported as unsupported -- preserving the exact fail-open coverage hole this pattern exists to close, and encoding that behaviour in the test. The lookaheads are removed and the three families are counted again. The test asserts the positive case for all three. Both this file's test and ptx_transform pass. The merge collision is real but belongs at merge time: when feature/sm120-exact-stage1 lands, the same commit removes the three prefixes it models from this pattern, so the hole is never open in between. That instruction is now in the comment beside the pattern rather than in a branch that has not merged. Test suite: the same four tests fail before and after (context_lifecycle, vmem_tuning, run_with_bpftime, mqsim_benchmark), all pre-existing in a CPU-only build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You are right and I have reverted it (66f5fb7). The trade I made was wrong in the direction that matters: The lookaheads are gone, all three families are counted again, and the test asserts the positive case for each. The merge instruction now lives in a comment beside the pattern instead of in an unmerged branch: when Same four pre-existing tests fail before and after ( |
The review is right. A PTX statement may be written across several physical
lines, and the unsupported scan matched one line at a time, so an asynchronous
copy split across two lines matched neither half and was never reported. If the
same kernel also held an ordinary global load, the module was still marked
instrumented, and the launch proceeded with an access nothing had recorded --
the fail-open case this scan exists to prevent.
Physical lines are now accumulated into one logical statement before the scan.
A statement is treated as open until its text, with any trailing comment
removed, ends in one of `;`, `{`, `}` or `:`. The rewrite path is untouched and
still matches per line; only the scan sees joined statements, so no rewritten
output changes.
The new case in ptx_async_copy_coverage_test writes the same bulk tensor copy
once across two lines and once on one line, and requires the same count from
both. It fails on the previous pass at the equality assertion and passes on
this one.
Test suite: 88% passed, the same four failing as before (context_lifecycle,
vmem_tuning, run_with_bpftime, mqsim_benchmark), all pre-existing in a CPU-only
build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed in 936b8e5. I reproduced the multi-line case before changing anything: a bulk tensor copy split after Physical lines are now accumulated into one logical statement before the scan. A statement counts as open until its text, with any trailing comment stripped, ends in The new test writes the same statement once across two lines and once on one line and requires an equal count. It fails on the previous pass at that equality and passes on this one. Suite: 88% passed, same four pre-existing failures. |
cp.asyncand the bulk tensor copy instructions read global memory, and the PTX pass matched them with neither of its two patterns. An HBF address reached by one produced no entry of any kind — not a modeled delay, and not an unsupported-list entry either. Reported as07-B-cp-async-and-bulk-tensor-copy-unmatched.md; independently reproduced here before fixing.Design goal this violates, line 27 of
docs/superpowers/specs/2026-08-09-hbfsim-hybrid-design.md:The change
Two alternatives added to the unsupported pattern in
src/ptxpass_hbf/transform.cpp, both requiring.globalin the opcode:The
.globalrequirement is deliberate. The same families carry pure synchronisation forms —cp.async.commit_group,cp.async.wait_group,cp.async.bulk.wait_group,cp.async.mbarrier.arrive— which touch no memory, and a shared-to-shared bulk copy names no global space. The test asserts each of those stays out of the count.The unsupported list reaches
src/cuda_runtime/coverage.cpp:379-396. Two outcomes:opaque_unmodeled_timingwithmodeled: false. Pure visibility gain; the access was already uncharged.unsupported_operation. A kernel using these instructions against a registered range that used to run will now be refused.That second one is what failing closed means, but Triton emits
cp.asyncfreely on recent architectures, so whether you want it on now is your call. If you would rather have visibility without refusals, say so and I will route these opcodes to the recorded-but-allowed path instead.Test
tests/cpu/ptx_async_copy_coverage_test.cpp— fails on the old pattern at the first asynchronous copy, passes on the new one.One trap worth noting for review: the probe kernel needs a
ld.paramto load its pointer, andld.paramis itself on the unsupported list. My first version of this test counted absolute totals, so every case looked unsupported and the test went green against unfixed code. Each case is now measured as an increment over the same kernel without the instruction under test. No case in the file rewrites an instruction, so it needs no embedded device helper and runs in a CPU-only build; rewriting cases stay inptx_transform_test.Test status
CPU-only build. Same four tests fail before and after —
context_lifecycle,vmem_tuning,run_with_bpftime,mqsim_benchmark— all pre-existing and unrelated.🤖 Generated with Claude Code