Check for gpu register crosstalk - #9325
Open
abadams wants to merge 13 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9325 +/- ##
==========================================
+ Coverage 70.11% 70.13% +0.02%
==========================================
Files 259 260 +1
Lines 79135 79264 +129
Branches 19286 19320 +34
==========================================
+ Hits 55487 55594 +107
- Misses 17880 17898 +18
- Partials 5768 5772 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
abadams
commented
Aug 13, 2026
alexreinking
approved these changes
Aug 14, 2026
alexreinking
requested changes
Aug 16, 2026
alexreinking
left a comment
alexreinking
left a comment
Member
There was a problem hiding this comment.
Need to fix the error test in Make
Register and stack memory is private to a GPU thread, so an allocation of one outside the loops over threads gives every thread its own copy of the whole thing rather than one copy shared by the block. A thread that reads a part of it another thread computed reads its own uninitialized copy of that part instead. Halide accepted such schedules and quietly computed the wrong answer. Check that each thread keeps to its own part of the allocation, in the manner of can_parallelize_rvar: describe the same access made by some other thread, and require that the two can never meet. Only the variables bound at or inside the loops over threads are renamed to make the other thread, because the rest, such as the base of the block's tile, are shared by the whole block. The check has to sit between storage folding and storage flattening, which is later in lowering than validation usually goes. See the comment in CheckGPUCrossTalk.h. Also lifts RenameFreeVars and the substitution of boolean lets out of ParallelRVar.cpp, which is where this technique already lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An allocation of thread-private memory outside the loops over threads is also broken when a store to it is not in as many loops over threads as the loads are. Fusing the loops leaves such a store guarded by a test that only the first thread of the missing dimensions passes, so every other thread reads its own copy of something it never wrote. A Func computed at the block level in registers hits this, which is an ordinary thing to write. Drop the exemption for stores that do not depend on which thread they are in. The loops over threads are unordered, so it does not make a store safe, and for a store outside them it is not even true that every thread does it. Review notes: indent the new Makefile entries, have RenameFreeVars make names that are unique rather than suffixed by hand, and expand chained boolean lets in one pass rather than substituting each through the rest. Test the cases that work as well as the ones that do not: one element per thread and several per thread, against a neighbour read, a scalar stored by one thread, a row stored by one warp, and a shifted one-to-one read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…isjoint Requiring each thread to keep to a disjoint part of the allocation is stronger than it needs to be. A tail strategy has neighbouring threads recompute each other's values, so their parts overlap, and each still only reads what it wrote itself. Ask for that instead: every load must be within a region this same thread has already stored. Three things that took a test each to find. The store has to come first in the body, or the load an update definition does of the site it is about to store to matches that very store. It has to be in at least as many loops over threads, or a store done by one thread stands in for all of them. And the region is taken with the thread symbolic but the loops within a thread's part bounded: bounding the thread widens every region to cover all of them, which is what we are trying to tell apart, and leaving the inner loops unbounded cannot describe a tile. Tail strategies wrap their clamp in a likely intrinsic, which has to come off before any of that folds. Test a tile per thread walked by serial loops with such a tail, and two stages that agree, against two stages that do not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A load has to be shown to be within the region the same thread stores, and the loops a thread runs inside its own part of the allocation are what bound how far its accesses reach. Only constant bounds were kept, so a loop that starts at the thread's own part of the allocation was dropped, its region came back unbounded, and the check failed. That happens as soon as anything is staged through a wrapper computed inside the thread loops. Keep the bounds whatever they are, and canonicalize them the way the accesses themselves are canonicalized, so that a bound naming one loop over threads and an access naming another are talking about the same thread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check was reformulated twice, and each formulation left something behind. The first one proved that no two threads could meet, which meant renaming one thread's variables to make a second instance of it, so the machinery for that was hoisted out of ParallelRVar.cpp. Reading a thread's own stores instead needs none of it, and nothing but ParallelRVar.cpp has used it since. Put those files back the way they were. Along the same lines, drop per_thread, which nothing reads, and the includes and helper that went with the formulations that are gone. An access no longer carries its own copy of the loops over threads it sits in. Its arguments are put in terms of the fused loops when it is recorded, which is all anything downstream wanted, along with how deep it sits and how many threads there are. Its position in the list is what says whether it has already happened, so it needs no separate ordering. Each access's region is now computed once rather than once per candidate partner, which is the difference between one simplifier pass per access per dimension and one per pair of accesses per dimension. Note in both places that decide what a loop over threads is why a loop over lanes counts for one and not the other: the lanes of a warp share registers, which is what makes warp shuffles work, but two threads share nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Substituting copies each let's value into every use, so a chain of lets that each build on the one before expands into something the size of their product. Wrap the definitions around the index instead. Everything the index is then put through - substitute, simplify, and bounds_of_expr_in_scope - already handles lets. The one place that wants them expanded is the description of an access in an error message, which only happens on the way to aborting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resolve() rewrapped the peeled lets by hand, testing each one against the body with expr_uses_var, which is quadratic in the number of lets. rewrap_used_lets does the same thing in one pass. An access in an error message is now printed the way it was written rather than with its lets expanded. Expanding them produces a bigger expression, not a clearer one, and the names it drops are the ones the schedule used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every place that put a let-bound index back together went on to rewrite it in terms of the fused loops over threads, so canonicalize does both, and as a member it needs neither the lets nor the loops passed to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check is not specific to CUDA, so the tests for it shouldn't be either. Use the GPU the environment names and skip when it names none, the way other GPU tests do. Verified that all five still report the error under OpenCL as well as CUDA, and that the one that should compile still does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An error test that skips produces no error, which the Makefile's test harness reads as a failure. These are compile-time checks, so they need a GPU API but not a GPU: name one when the environment doesn't, and the check gets tested everywhere rather than only where a GPU is attached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An error test that returns cleanly produces no error, which the Makefile's test harness reads as a failure. Skip the way the other error tests that can't always run do, with an assert to report something. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
abadams
force-pushed
the
abadams/gpu_register_crosstalk_check
branch
from
August 16, 2026 22:21
65b8f0b to
74fa54b
Compare
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
2 participants
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.
It's currently possible to compute something at the gpu block level but store it in registers. This is a problem, because in general you can't read registers written by another thread (unless it's a warp shuffle). These allocations were silently thread-local instead of actually shared, so it's currently possible to write a schedule that produces garbage this way. We could just forbid using this memory type at the block level, but the canonical CUDA sgemm does indeed use register storage at the block scope to store its accumulators, so forbidding it would make that schedule impossible.
This PR instead adds a pass that tries to check it's safe, in the spirit of the way in which we allow parallelization of some rvars, if it can be proved hazard-free.
I tried and failed to make this a property of or a check on the schedule. The robust place to do it turned out to be early in lowering after you have some loops nests stamped down.