From 3a562c82b266cfcf1e7069c212cc35b3a791aacc Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 21 Aug 2026 11:52:13 -0700 Subject: [PATCH 1/8] Let register storage sit outside a loop over GPU threads validate_schedule rejects any parallel loop between where a Func is stored and where it is computed, because the iterations would write the same storage. A loop over GPU threads does not, when the storage is Register: a thread's registers are its own, so each thread gets a copy rather than sharing one, which is what the memory type means. Exempt that one case. What the exemption does not establish is that each thread then keeps to its own copy, and it does not have to, because check_gpu_cross_talk already answers exactly that question, later in lowering, once storage folding has had its say. Its error message is about this configuration in as many words - storage "scheduled outside the loops over GPU threads, so every thread gets its own copy of it rather than sharing one". Being rejected up front is what stopped such schedules from ever reaching it. Both sides are tested, because the interesting property is the division of labour between the two checks rather than either alone. The correctness test is a schedule that is fine and was refused; the error test is one that is not fine, gets past the validator now, and has to be stopped by the cross-talk check. Without this change both fail at the validator, the second for the wrong reason. The five existing cross-talk error tests all compute at the block level, so none of them has a thread loop between store and compute - this configuration could not be reached before, and so was not covered. Co-authored-by: Claude Opus 5 --- src/ScheduleFunctions.cpp | 21 ++++++-- test/correctness/CMakeLists.txt | 1 + ...pu_register_stored_outside_thread_loop.cpp | 52 +++++++++++++++++++ test/error/CMakeLists.txt | 1 + ...r_crosstalk_stored_outside_thread_loop.cpp | 47 +++++++++++++++++ 5 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 test/correctness/gpu_register_stored_outside_thread_loop.cpp create mode 100644 test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp diff --git a/src/ScheduleFunctions.cpp b/src/ScheduleFunctions.cpp index e5e5efc310cc..bcd28f035666 100644 --- a/src/ScheduleFunctions.cpp +++ b/src/ScheduleFunctions.cpp @@ -1989,7 +1989,7 @@ class InjectFunctionRealization : public IRMutator { class ComputeLegalSchedules : public IRVisitor { public: struct Site { - bool is_parallel, is_gpu_block; + bool is_parallel, is_gpu_block, is_gpu_thread; LoopLevel loop_level; }; vector sites_allowed; @@ -2026,7 +2026,9 @@ class ComputeLegalSchedules : public IRVisitor { // thus any new ones we synthesize we must explicitly lock. loop_level.lock(); const bool is_gpu_block = (f->for_type == ForType::GPUBlock); - sites.push_back({f->is_parallel(), is_gpu_block, loop_level}); + const bool is_gpu_thread = (f->for_type == ForType::GPUThread || + f->for_type == ForType::GPULane); + sites.push_back({f->is_parallel(), is_gpu_block, is_gpu_thread, loop_level}); f->min.accept(this); f->max.accept(this); @@ -2452,10 +2454,21 @@ bool validate_schedule(Function f, const Stmt &s, const Target &target, bool is_ return store_idx >= 0 && compute_idx >= 0 && hoist_storage_idx >= 0; }; + // Storage private to a GPU thread is not shared by a loop over threads, so + // such a loop between where it is stored and where it is computed is not a + // race - each thread gets its own copy, which is what the memory type + // means. Whether each thread then keeps to its own copy is a different + // question, and check_gpu_cross_talk answers it later in lowering, once + // storage folding has had its say. + const bool thread_private = f.schedule().memory_type() == MemoryType::Register; + const auto races = [&](int i) { + return sites[i].is_parallel && !(thread_private && sites[i].is_gpu_thread); + }; + // Check there isn't a parallel loop between the compute_at and the store_at if (all_ok()) { for (int i = store_idx + 1; i <= compute_idx; i++) { - if (sites[i].is_parallel) { + if (races(i)) { err << "Func \"" << f.name() << "\" is stored outside the parallel/vectorized/gpu_block loop over " << sites[i].loop_level.to_string() @@ -2468,7 +2481,7 @@ bool validate_schedule(Function f, const Stmt &s, const Target &target, bool is_ // Check there isn't a parallel loop between the compute_at and the hoist_storage_at if (all_ok()) { for (int i = hoist_storage_idx + 1; i <= compute_idx; i++) { - if (sites[i].is_parallel) { + if (races(i)) { err << "Func \"" << f.name() << "\" storage is hoisted outside the parallel/vectorized/gpu_block loop over " << sites[i].loop_level.to_string() diff --git a/test/correctness/CMakeLists.txt b/test/correctness/CMakeLists.txt index 5ca439110784..491f35f178b9 100644 --- a/test/correctness/CMakeLists.txt +++ b/test/correctness/CMakeLists.txt @@ -176,6 +176,7 @@ tests( gpu_object_lifetime_3.cpp gpu_param_allocation.cpp gpu_register_at_block_level.cpp + gpu_register_stored_outside_thread_loop.cpp gpu_reuse_shared_memory.cpp gpu_specialize.cpp gpu_store_in_register_with_no_lanes_loop.cpp diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp new file mode 100644 index 000000000000..5c83ffd787b2 --- /dev/null +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -0,0 +1,52 @@ +#include "Halide.h" +#include + +using namespace Halide; + +// Register memory is private to a GPU thread, so a loop over threads between +// where it is stored and where it is computed is not a race: each thread gets +// its own copy rather than sharing one. Storing at the block level and +// computing within the threads is therefore allowed, and what makes it safe is +// that each thread only ever touches the part of its own copy that it wrote. +// That is check_gpu_cross_talk's business, not the schedule validator's - see +// error/gpu_register_crosstalk_stored_outside_thread_loop.cpp for the other +// side of it. + +int main(int argc, char **argv) { + Target target = get_jit_target_from_environment(); + if (!target.has_gpu_feature()) { + printf("[SKIP] No GPU target enabled.\n"); + return 0; + } + + Func f("f"), g("g"); + Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); + + f(x, y) = x + y * 1000; + g(x, y) = f(x, y) * 2; + + g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); + + // Stored outside the loops over threads, computed inside them. Each thread + // writes and reads only its own (x, y). + f.store_at(g, xo) + .compute_at(g, xi) + .store_in(MemoryType::Register); + + Buffer result = g.realize({64, 64}, target); + result.copy_to_host(); + + for (int y = 0; y < result.height(); y++) { + for (int x = 0; x < result.width(); x++) { + int correct = (x + y * 1000) * 2; + if (result(x, y) != correct) { + printf("result(%d, %d) = %d instead of %d\n", + x, y, result(x, y), correct); + return 1; + } + } + } + + printf("Success!\n"); + return 0; +} diff --git a/test/error/CMakeLists.txt b/test/error/CMakeLists.txt index 5ff9f97d944d..950bfc813a5f 100644 --- a/test/error/CMakeLists.txt +++ b/test/error/CMakeLists.txt @@ -74,6 +74,7 @@ tests( fuse_same_var.cpp fuse_vectorized_var_with_rvar.cpp gpu_register_crosstalk.cpp + gpu_register_crosstalk_stored_outside_thread_loop.cpp gpu_register_shifted_between_threads.cpp gpu_register_stages_disagree.cpp gpu_register_stored_by_one_thread.cpp diff --git a/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp b/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp new file mode 100644 index 000000000000..24e66511c920 --- /dev/null +++ b/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp @@ -0,0 +1,47 @@ +#include "Halide.h" +#include + +using namespace Halide; + +// The schedule validator lets register storage sit outside a loop over GPU +// threads, because a thread's own registers are not shared with any other +// thread, so such a loop between where it is stored and where it is computed +// is not a race. What that does not establish is that each thread then keeps +// to its own copy, and check_gpu_cross_talk is what establishes it, later in +// lowering. This is a schedule that gets past the first and has to be stopped +// by the second. +// +// See correctness/gpu_register_stored_outside_thread_loop.cpp for the same +// placement without the cross-talk. + +int main(int argc, char **argv) { + Target target = get_jit_target_from_environment(); + if (!target.has_gpu_feature()) { + printf("[SKIP] No GPU target enabled.\n"); + // An error test has to report an error even when it skips. + _halide_user_assert(0); + } + + Func f("f"), g("g"); + Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); + + f(x, y) = x + y * 1000; + // The second term is the value the neighbouring thread computed. + g(x, y) = f(x, y) + f(x - 1, y); + + g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); + + // Stored at the block level, computed within the loop over threads in y, + // and spread across the threads in x by its own gpu_threads. So no thread + // holds the whole of f, but each has its own copy, and the value a thread + // wants from its neighbour is not in the copy it has. + f.store_at(g, xo) + .compute_at(g, yi) + .store_in(MemoryType::Register) + .gpu_threads(x); + + g.compile_jit(target); + + printf("Success!\n"); + return 0; +} From 68d81ddc6c690bb35f6195252bff5cb621b719dd Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 21 Aug 2026 11:55:22 -0700 Subject: [PATCH 2/8] Cut a wrong reason from the cross-talk comment Storage folding is not why the cross-talk check runs where it does: it refuses to descend into a parallel loop at all, so it cannot rewrite these accesses. The check runs late because the whole loop nest is in place by then, and anticipating what every part of the schedule will do to it would be much harder. Co-authored-by: Claude Opus 5 --- src/ScheduleFunctions.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ScheduleFunctions.cpp b/src/ScheduleFunctions.cpp index bcd28f035666..b3610959e746 100644 --- a/src/ScheduleFunctions.cpp +++ b/src/ScheduleFunctions.cpp @@ -2458,8 +2458,7 @@ bool validate_schedule(Function f, const Stmt &s, const Target &target, bool is_ // such a loop between where it is stored and where it is computed is not a // race - each thread gets its own copy, which is what the memory type // means. Whether each thread then keeps to its own copy is a different - // question, and check_gpu_cross_talk answers it later in lowering, once - // storage folding has had its say. + // question, and check_gpu_cross_talk answers it later in lowering. const bool thread_private = f.schedule().memory_type() == MemoryType::Register; const auto races = [&](int i) { return sites[i].is_parallel && !(thread_private && sites[i].is_gpu_thread); From 9677b15de784c6f28d18bd722218a1066fe053e6 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 21 Aug 2026 12:04:51 -0700 Subject: [PATCH 3/8] Fold the cross-talk error case into the correctness test Uses expect_user_error rather than a test/error file, which is where these are heading. The two cases belong together anyway: what is being tested is the division of labour between the schedule validator and the cross-talk check, and either case alone only shows half of it. Matching on the message matters here rather than just on erroring at all, because before this change the schedule was rejected too - by the validator, for a different reason. Reaching the error only takes compiling, so that half runs on a machine with no GPU, which the test/error version could not do. The two halves skip independently and say which one they skipped: without exceptions there is no error to catch, and without a device there is nothing to run. Co-authored-by: Claude Opus 5 --- ...pu_register_stored_outside_thread_loop.cpp | 92 +++++++++++++++---- test/error/CMakeLists.txt | 1 - ...r_crosstalk_stored_outside_thread_loop.cpp | 47 ---------- 3 files changed, 74 insertions(+), 66 deletions(-) delete mode 100644 test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index 5c83ffd787b2..cee3961c00fe 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -1,24 +1,26 @@ +// Register memory is private to a GPU thread, so a loop over threads between +// where a Func is stored and where it is computed is not a race: each thread +// gets its own copy rather than sharing one. Storing at the block level and +// computing within the threads is therefore allowed. +// +// What makes such a schedule safe is that each thread only ever touches the +// part of its own copy that it wrote, and that is check_gpu_cross_talk's +// question rather than the schedule validator's. So the two halves below are a +// pair: a placement that is fine and used to be refused up front, and one that +// is not fine, gets past the validator, and has to be caught by the cross-talk +// check instead. + #include "Halide.h" +#include "expect_user_error.h" #include using namespace Halide; -// Register memory is private to a GPU thread, so a loop over threads between -// where it is stored and where it is computed is not a race: each thread gets -// its own copy rather than sharing one. Storing at the block level and -// computing within the threads is therefore allowed, and what makes it safe is -// that each thread only ever touches the part of its own copy that it wrote. -// That is check_gpu_cross_talk's business, not the schedule validator's - see -// error/gpu_register_crosstalk_stored_outside_thread_loop.cpp for the other -// side of it. - -int main(int argc, char **argv) { - Target target = get_jit_target_from_environment(); - if (!target.has_gpu_feature()) { - printf("[SKIP] No GPU target enabled.\n"); - return 0; - } +namespace { +// Each thread computes and reads only its own (x, y), so no thread depends on +// a value another one was responsible for. +int keeps_to_its_own_copy(const Target &target) { Func f("f"), g("g"); Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); @@ -26,9 +28,6 @@ int main(int argc, char **argv) { g(x, y) = f(x, y) * 2; g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); - - // Stored outside the loops over threads, computed inside them. Each thread - // writes and reads only its own (x, y). f.store_at(g, xo) .compute_at(g, xi) .store_in(MemoryType::Register); @@ -46,6 +45,63 @@ int main(int argc, char **argv) { } } } + return 0; +} + +#if HALIDE_WITH_EXCEPTIONS + +// Reaching the error only needs compiling, so this half runs without a device. +Target compile_only_target() { + return get_host_target() + .with_feature(Target::CUDA) + .with_feature(Target::CUDACapability80); +} + +// Stored at the block level, computed within the loop over threads in y, and +// spread across the threads in x by its own gpu_threads. No thread holds the +// whole of f, and the value each one wants from its neighbour is not in the +// copy it has. +void reads_another_threads_copy() { + Func f("f"), g("g"); + Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); + + f(x, y) = x + y * 1000; + g(x, y) = f(x, y) + f(x - 1, y); + + g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); + f.store_at(g, xo) + .compute_at(g, yi) + .store_in(MemoryType::Register) + .gpu_threads(x); + + g.compile_jit(compile_only_target()); +} + +#endif // HALIDE_WITH_EXCEPTIONS + +} // namespace + +int main(int argc, char **argv) { +#if HALIDE_WITH_EXCEPTIONS + if (!expect_user_error("reads_another_threads_copy", + "keeps to its own part", + reads_another_threads_copy)) { + return 1; + } +#else + printf("[SKIP] Halide was compiled without exceptions, so the schedule " + "that must be rejected is not exercised.\n"); +#endif + + Target target = get_jit_target_from_environment(); + if (!target.has_gpu_feature()) { + printf("[SKIP] No GPU target enabled, so the schedule that must be " + "accepted is not exercised.\n"); + return 0; + } + if (keeps_to_its_own_copy(target) != 0) { + return 1; + } printf("Success!\n"); return 0; diff --git a/test/error/CMakeLists.txt b/test/error/CMakeLists.txt index 950bfc813a5f..5ff9f97d944d 100644 --- a/test/error/CMakeLists.txt +++ b/test/error/CMakeLists.txt @@ -74,7 +74,6 @@ tests( fuse_same_var.cpp fuse_vectorized_var_with_rvar.cpp gpu_register_crosstalk.cpp - gpu_register_crosstalk_stored_outside_thread_loop.cpp gpu_register_shifted_between_threads.cpp gpu_register_stages_disagree.cpp gpu_register_stored_by_one_thread.cpp diff --git a/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp b/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp deleted file mode 100644 index 24e66511c920..000000000000 --- a/test/error/gpu_register_crosstalk_stored_outside_thread_loop.cpp +++ /dev/null @@ -1,47 +0,0 @@ -#include "Halide.h" -#include - -using namespace Halide; - -// The schedule validator lets register storage sit outside a loop over GPU -// threads, because a thread's own registers are not shared with any other -// thread, so such a loop between where it is stored and where it is computed -// is not a race. What that does not establish is that each thread then keeps -// to its own copy, and check_gpu_cross_talk is what establishes it, later in -// lowering. This is a schedule that gets past the first and has to be stopped -// by the second. -// -// See correctness/gpu_register_stored_outside_thread_loop.cpp for the same -// placement without the cross-talk. - -int main(int argc, char **argv) { - Target target = get_jit_target_from_environment(); - if (!target.has_gpu_feature()) { - printf("[SKIP] No GPU target enabled.\n"); - // An error test has to report an error even when it skips. - _halide_user_assert(0); - } - - Func f("f"), g("g"); - Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); - - f(x, y) = x + y * 1000; - // The second term is the value the neighbouring thread computed. - g(x, y) = f(x, y) + f(x - 1, y); - - g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); - - // Stored at the block level, computed within the loop over threads in y, - // and spread across the threads in x by its own gpu_threads. So no thread - // holds the whole of f, but each has its own copy, and the value a thread - // wants from its neighbour is not in the copy it has. - f.store_at(g, xo) - .compute_at(g, yi) - .store_in(MemoryType::Register) - .gpu_threads(x); - - g.compile_jit(target); - - printf("Success!\n"); - return 0; -} From 713ed325d192c278fd508b04f38315f28a8bcf80 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 21 Aug 2026 14:17:26 -0700 Subject: [PATCH 4/8] Let the cross-talk check see past dimensions that don't separate threads The placement this branch allows is only worth having because a serial loop can sit between the storage and the loop over threads, and a producer can slide over that loop while staying computed inside the threads. The test now schedules that, rather than a placement with nothing in between, which was permitted but pointless - it allocated the whole block's worth per thread so each could write one row of it. Scheduling the real thing shows the check refusing it. A slid producer loads what the previous run of the loop stored, and the check looked only at stores earlier in the list, so the load it complained about was the first access there was. Two things let it through. A dimension whose region is the same whatever thread is asking is one every thread walks identically, so a coordinate a load names in it is one every thread names, and it cannot be what makes a load another thread's - leave those out of the comparison. It is the region that has to be asked and not the index: a loop of a thread's own is written the same way by every thread, and only its bounds say which part is whose. Then a store listed after a load counts when the two differ along such a dimension, because that is what carries an allocation from one run of a loop to the next. Where every dimension agrees there is no gap to carry anything, and a later store is simply later, which is what gpu_register_stages_disagree relies on. Co-authored-by: Claude Opus 5 --- src/CheckGPUCrossTalk.cpp | 46 +++++++++- ...pu_register_stored_outside_thread_loop.cpp | 87 +++++++++++++------ 2 files changed, 105 insertions(+), 28 deletions(-) diff --git a/src/CheckGPUCrossTalk.cpp b/src/CheckGPUCrossTalk.cpp index 73f2a4ecfb6f..3e2c51a0272f 100644 --- a/src/CheckGPUCrossTalk.cpp +++ b/src/CheckGPUCrossTalk.cpp @@ -2,7 +2,9 @@ #include "Bounds.h" #include "CanonicalizeGPUVars.h" +#include "ExprUsesVar.h" #include "IR.h" +#include "IREquality.h" #include "IROperator.h" #include "IRPrinter.h" #include "IRVisitor.h" @@ -245,6 +247,28 @@ class CheckCrossTalk : public IRVisitor { } } + // Which dimensions tell one thread's part from another's. A + // dimension whose region is the same whatever thread is asking is one + // every thread walks the same way, so a coordinate a load names in it + // is one every thread names. Such a dimension cannot be what makes a + // load another thread's. It is the region that has to be asked, not + // the index: a loop of a thread's own is written the same way by every + // thread, and it is its bounds that say which part is whose. + vector separates_threads(finder.accesses[0].args.size(), false); + for (const auto ®ion : regions) { + for (size_t i = 0; i < region.size(); i++) { + for (int t = 0; t < 3; t++) { + const string &n = gpu_thread_name(t); + separates_threads[i] = + separates_threads[i] || + (region[i].has_lower_bound() && + stmt_or_expr_uses_var(region[i].min, n)) || + (region[i].has_upper_bound() && + stmt_or_expr_uses_var(region[i].max, n)); + } + } + } + for (size_t l = 0; l < finder.accesses.size(); l++) { const Access &load = finder.accesses[l]; if (load.is_store) { @@ -257,15 +281,35 @@ class CheckCrossTalk : public IRVisitor { // no other thread stored this one, this thread did. A site this // thread never wrote holds a value nothing depends on, like the // garbage that pads out a vector. - for (size_t s = 0; s < l && !ok; s++) { + // Stores earlier in the list have happened. A later one still + // counts if it lands somewhere else along a dimension that does + // not separate threads, because such a dimension is what carries + // an allocation from one run of a loop to the next: the store this + // load wants is the one the previous iteration ran, and the thread + // that ran it was this one. Where every dimension agrees there is + // no such gap, and a later store is simply later. + for (size_t s = 0; s < finder.accesses.size() && !ok; s++) { const Access &store = finder.accesses[s]; // The store has to be in at least as many loops over threads, // or it is the work of one thread standing in for all of them. if (!store.is_store || store.thread_depth < load.thread_depth) { continue; } + if (s > l) { + bool carried = false; + for (size_t i = 0; i < regions[l].size() && !carried; i++) { + carried = (!separates_threads[i] && + !equal(load.canonical_args[i], store.canonical_args[i])); + } + if (!carried) { + continue; + } + } ok = true; for (size_t i = 0; i < regions[l].size() && ok; i++) { + if (!separates_threads[i]) { + continue; + } const Interval &want = regions[l][i], &have = regions[s][i]; ok = (want.has_lower_bound() && want.has_upper_bound() && have.has_lower_bound() && have.has_upper_bound() && diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index cee3961c00fe..77886a409f8a 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -1,14 +1,19 @@ // Register memory is private to a GPU thread, so a loop over threads between // where a Func is stored and where it is computed is not a race: each thread -// gets its own copy rather than sharing one. Storing at the block level and -// computing within the threads is therefore allowed. +// gets its own copy rather than sharing one. // -// What makes such a schedule safe is that each thread only ever touches the -// part of its own copy that it wrote, and that is check_gpu_cross_talk's -// question rather than the schedule validator's. So the two halves below are a -// pair: a placement that is fine and used to be refused up front, and one that -// is not fine, gets past the validator, and has to be caught by the cross-talk -// check instead. +// The point of such a placement is not the loop over threads, which nothing +// can slide or fold over. It is that a serial loop can sit in there too. A +// group of warps cooperates on a walk over a serial reduction, and a producer +// feeding it slides over that walk while staying computed inside the warps, so +// its values never have to leave registers to cross between them. That needs +// the storage above the walk, which puts it above the loop over warps as well. +// +// What makes it safe is that each warp only ever touches the rows of its own +// copy that it wrote, and that is check_gpu_cross_talk's question rather than +// the schedule validator's. So the two halves below are a pair: a placement +// that is fine and used to be refused up front, and one that is not fine, gets +// past the validator, and has to be caught by the cross-talk check instead. #include "Halide.h" #include "expect_user_error.h" @@ -18,29 +23,57 @@ using namespace Halide; namespace { -// Each thread computes and reads only its own (x, y), so no thread depends on -// a value another one was responsible for. -int keeps_to_its_own_copy(const Target &target) { - Func f("f"), g("g"); - Var x("x"), y("y"), xo("xo"), yo("yo"), xi("xi"), yi("yi"); - - f(x, y) = x + y * 1000; - g(x, y) = f(x, y) * 2; - - g.gpu_tile(x, y, xo, yo, xi, yi, 16, 16); - f.store_at(g, xo) - .compute_at(g, xi) +// The width of the accumulator, how many rows there are, how many warps share +// them, and how many steps the walk takes. +const int W = 8, Y = 64, WARPS = 4, N = 16; +const int rows_per_warp = Y / WARPS; + +// A producer stored above a serial walk, computed within the loop over warps +// inside it, and slid over the walk. Each warp writes and reads only its own +// rows, and only the two steps of the walk that are live. +int slid_over_a_walk_inside_the_warps(const Target &target) { + Func p("p"), acc("acc"), out("out"); + Var x("x"), y("y"), t("t"), yo("yo"), yw("yw"), yi("yi"); + RDom rt(0, N, "rt"); + + p(y, t) = cast((y + 1) * (t + 1)); + + // Each step needs two consecutive values of p, so p slides over the walk. + acc(x, y) = 0.f; + acc(x, y) += (p(y, rt) - p(y, rt - 1)) * cast(x + 1); + + out(x, y) = acc(x, y); + + out.bound(x, 0, W).bound(y, 0, Y).compute_root(); + out.split(y, yo, yw, rows_per_warp).gpu_blocks(yo).gpu_threads(yw); + + acc.compute_at(out, yo) + .split(y, yw, yi, rows_per_warp) + .gpu_threads(yw); + // The walk is outside the loop over warps, so the warps run it together. + acc.update() + .split(y, yw, yi, rows_per_warp) + .reorder(x, yi, yw, rt) + .gpu_threads(yw); + + // Stored above the walk - and so above the loop over warps - computed + // within the warps, and folded down to the two steps that are live. + p.store_at(out, yo) + .compute_at(acc, yw) .store_in(MemoryType::Register); - Buffer result = g.realize({64, 64}, target); + Buffer result = out.realize({W, Y}, target); result.copy_to_host(); - for (int y = 0; y < result.height(); y++) { - for (int x = 0; x < result.width(); x++) { - int correct = (x + y * 1000) * 2; + // p(y, rt) - p(y, rt - 1) is y + 1 at every step, so each row accumulates + // that times the column, once per step. A warp that read another warp's + // rows would get a different row's answer. + for (int y = 0; y < Y; y++) { + for (int x = 0; x < W; x++) { + float correct = (float)N * (float)(y + 1) * (float)(x + 1); if (result(x, y) != correct) { - printf("result(%d, %d) = %d instead of %d\n", - x, y, result(x, y), correct); + printf("result(%d, %d) = %f instead of %f\n", + x, y, (double)result(x, y), (double)correct); return 1; } } @@ -99,7 +132,7 @@ int main(int argc, char **argv) { "accepted is not exercised.\n"); return 0; } - if (keeps_to_its_own_copy(target) != 0) { + if (slid_over_a_walk_inside_the_warps(target) != 0) { return 1; } From 42af48de91fe17586a9676b93b69ac07c4817807 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 21 Aug 2026 15:11:20 -0700 Subject: [PATCH 5/8] Ask who wrote a site last, not whether this thread wrote it at all Skipping the dimensions that don't separate threads was not enough on its own, and made the check accept two schedules it used to reject, both with silently wrong answers. Both are a slice added to an existing error test: one leaves a stage serial so a single thread writes the slice on everyone's behalf, the other maps that stage's threads the other way round so the slice is written by the thread with its coordinates transposed. In both, the load of the second slice was excused by the store to the first, because the dimension that told the slices apart was one of the ones being skipped. What a thread reads is whatever was written to the site last, so finding one store of its own that covers the load says nothing if some other store could have landed there afterwards. A store is only somebody else's business if it cannot reach the site at all. So exonerate a store only when it runs in at least as many loops over threads as the load and covers it along the dimensions that separate threads, and require every other store to be provably disjoint from it. The same question was already being asked too weakly before any of this: a Func with a real pure definition and one stage left serial is accepted today and computes the wrong thing, because the pure definition's store covers everything and satisfies "this thread wrote it at some point". Asking about the last writer instead rejects that too. A dimension nothing could be bounded in now counts as separating, so failing to work out where an access reaches stays an error rather than excusing the dimension from the comparison. Co-authored-by: Claude Opus 5 --- src/CheckGPUCrossTalk.cpp | 68 +++++++++++++++---- ...pu_register_stored_outside_thread_loop.cpp | 60 +++++++++++++++- 2 files changed, 112 insertions(+), 16 deletions(-) diff --git a/src/CheckGPUCrossTalk.cpp b/src/CheckGPUCrossTalk.cpp index 3e2c51a0272f..0ae58b660713 100644 --- a/src/CheckGPUCrossTalk.cpp +++ b/src/CheckGPUCrossTalk.cpp @@ -254,21 +254,43 @@ class CheckCrossTalk : public IRVisitor { // load another thread's. It is the region that has to be asked, not // the index: a loop of a thread's own is written the same way by every // thread, and it is its bounds that say which part is whose. + // A dimension nothing could be bounded in stays in, so that failing + // to work out where an access reaches is still an error rather than a + // dimension that gets to sit the comparison out. vector separates_threads(finder.accesses[0].args.size(), false); for (const auto ®ion : regions) { for (size_t i = 0; i < region.size(); i++) { + if (!region[i].has_lower_bound() || !region[i].has_upper_bound()) { + separates_threads[i] = true; + continue; + } for (int t = 0; t < 3; t++) { const string &n = gpu_thread_name(t); separates_threads[i] = separates_threads[i] || - (region[i].has_lower_bound() && - stmt_or_expr_uses_var(region[i].min, n)) || - (region[i].has_upper_bound() && - stmt_or_expr_uses_var(region[i].max, n)); + stmt_or_expr_uses_var(region[i].min, n) || + stmt_or_expr_uses_var(region[i].max, n); } } } + // Does store s reach every site load l does, along the dimensions + // that tell one thread's part from another's? + const auto covers = [&](size_t s, size_t l) { + for (size_t i = 0; i < regions[l].size(); i++) { + if (!separates_threads[i]) { + continue; + } + const Interval &want = regions[l][i], &have = regions[s][i]; + if (!(want.has_lower_bound() && want.has_upper_bound() && + have.has_lower_bound() && have.has_upper_bound() && + can_prove(have.min <= want.min && want.max <= have.max))) { + return false; + } + } + return true; + }; + for (size_t l = 0; l < finder.accesses.size(); l++) { const Access &load = finder.accesses[l]; if (load.is_store) { @@ -305,16 +327,36 @@ class CheckCrossTalk : public IRVisitor { continue; } } - ok = true; - for (size_t i = 0; i < regions[l].size() && ok; i++) { - if (!separates_threads[i]) { - continue; - } - const Interval &want = regions[l][i], &have = regions[s][i]; - ok = (want.has_lower_bound() && want.has_upper_bound() && - have.has_lower_bound() && have.has_upper_bound() && - can_prove(have.min <= want.min && want.max <= have.max)); + ok = covers(s, l); + } + // Finding one store of this thread's that covers the load is + // not enough. What a thread reads is what was written to the site + // last, so any store that might land on the same site has to be + // this thread's too. A store that cannot reach the site is no + // one's business, which is what the overlap test asks. + // + // Two ways a store that reaches it belongs to someone else: it + // runs in fewer loops over threads than the load, so one thread + // ran it on everyone's behalf; or it reaches the site from a + // different thread, which is what failing to cover the load along + // the dimensions that separate threads means. + for (size_t s = 0; s < finder.accesses.size() && ok; s++) { + const Access &store = finder.accesses[s]; + if (!store.is_store) { + continue; + } + if (store.thread_depth >= load.thread_depth && covers(s, l)) { + continue; + } + bool disjoint = false; + for (size_t i = 0; i < regions[l].size() && !disjoint; i++) { + const Interval &a = regions[l][i], &b = regions[s][i]; + disjoint = ((a.has_upper_bound() && b.has_lower_bound() && + can_prove(a.max < b.min)) || + (b.has_upper_bound() && a.has_lower_bound() && + can_prove(b.max < a.min))); } + ok = disjoint; } if (!ok) { report(op, finder.accesses, load); diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index 77886a409f8a..3a956b04af70 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -110,15 +110,69 @@ void reads_another_threads_copy() { g.compile_jit(compile_only_target()); } +// Finding one store of this thread's that covers a load is not enough: what a +// thread reads is whatever was written to the site last. Here the c == 1 slice +// is written by a stage with no loops over threads of its own, so one thread +// writes it on everyone's behalf, while the c == 0 slice is written by all of +// them. c is a constant in every access, so it cannot be what tells one +// thread's part from another's, and the load of c == 1 must not be excused by +// the store to c == 0. +void one_thread_stands_in_for_all_in_a_slice() { + Func f("f"), g("g"); + Var x("x"), y("y"), c("c"), xi("xi"), yi("yi"); + + f(x, y, c) = undef(); + f(x, y, 0) = x + y * 1000; + f(x, y, 1) = x + y * 1000 + 7; + g(x, y) = f(x, y, 0) + f(x, y, 1); + + g.gpu_tile(x, y, x, y, xi, yi, 16, 16); + f.compute_at(g, x).store_in(MemoryType::Register).bound(c, 0, 2); + f.update(0).gpu_threads(x, y); + // f.update(1) is left serial, so one thread runs all of it. + + g.compile_jit(compile_only_target()); +} + +// The same, with the stage that disagrees running in as many loops over +// threads as the load but mapping them the other way round, so the site a +// thread reads in the c == 1 slice was written by the thread with its +// coordinates transposed. +void stages_disagree_within_a_slice() { + Func f("f"), g("g"); + Var x("x"), y("y"), c("c"), xi("xi"), yi("yi"); + + f(x, y, c) = undef(); + f(x, y, 0) = x + y * 1000; + f(x, y, 1) = x + y * 1000 + 7; + g(x, y) = f(x, y, 0) + f(x, y, 1); + + g.gpu_tile(x, y, x, y, xi, yi, 16, 16); + f.compute_at(g, x).store_in(MemoryType::Register).bound(c, 0, 2); + f.update(0).gpu_threads(x, y); + f.update(1).reorder(y, x).gpu_threads(y, x); + + g.compile_jit(compile_only_target()); +} + #endif // HALIDE_WITH_EXCEPTIONS } // namespace int main(int argc, char **argv) { #if HALIDE_WITH_EXCEPTIONS - if (!expect_user_error("reads_another_threads_copy", - "keeps to its own part", - reads_another_threads_copy)) { + int failures = 0; + failures += !expect_user_error("reads_another_threads_copy", + "keeps to its own part", + reads_another_threads_copy); + failures += !expect_user_error("one_thread_stands_in_for_all_in_a_slice", + "keeps to its own part", + one_thread_stands_in_for_all_in_a_slice); + failures += !expect_user_error("stages_disagree_within_a_slice", + "keeps to its own part", + stages_disagree_within_a_slice); + if (failures != 0) { + printf("%d schedule(s) did not produce the expected user error\n", failures); return 1; } #else From 9b1c5bc8ab773a0a979b17f1c47d2becdd9e69bd Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Tue, 25 Aug 2026 13:02:12 -0700 Subject: [PATCH 6/8] Allow stack storage across a loop over GPU threads too Stack memory is private to a thread in the same way register memory is, and check_gpu_cross_talk already checks both of them for a thread reading a part another thread wrote. Letting only one of the two through here left the schedule rejected for a memory type that is just as safe. Co-Authored-By: Claude Opus 5 --- src/ScheduleFunctions.cpp | 7 +++++-- .../gpu_register_stored_outside_thread_loop.cpp | 16 ++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/ScheduleFunctions.cpp b/src/ScheduleFunctions.cpp index b3610959e746..e9e7b7dfa01f 100644 --- a/src/ScheduleFunctions.cpp +++ b/src/ScheduleFunctions.cpp @@ -2458,8 +2458,11 @@ bool validate_schedule(Function f, const Stmt &s, const Target &target, bool is_ // such a loop between where it is stored and where it is computed is not a // race - each thread gets its own copy, which is what the memory type // means. Whether each thread then keeps to its own copy is a different - // question, and check_gpu_cross_talk answers it later in lowering. - const bool thread_private = f.schedule().memory_type() == MemoryType::Register; + // question, and check_gpu_cross_talk answers it later in lowering, for + // these same two memory types. + const MemoryType mem = f.schedule().memory_type(); + const bool thread_private = + mem == MemoryType::Register || mem == MemoryType::Stack; const auto races = [&](int i) { return sites[i].is_parallel && !(thread_private && sites[i].is_gpu_thread); }; diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index 3a956b04af70..baa375f62402 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -31,7 +31,7 @@ const int rows_per_warp = Y / WARPS; // A producer stored above a serial walk, computed within the loop over warps // inside it, and slid over the walk. Each warp writes and reads only its own // rows, and only the two steps of the walk that are live. -int slid_over_a_walk_inside_the_warps(const Target &target) { +int slid_over_a_walk_inside_the_warps(const Target &target, MemoryType mem) { Func p("p"), acc("acc"), out("out"); Var x("x"), y("y"), t("t"), yo("yo"), yw("yw"), yi("yi"); RDom rt(0, N, "rt"); @@ -60,7 +60,7 @@ int slid_over_a_walk_inside_the_warps(const Target &target) { // within the warps, and folded down to the two steps that are live. p.store_at(out, yo) .compute_at(acc, yw) - .store_in(MemoryType::Register); + .store_in(mem); Buffer result = out.realize({W, Y}, target); result.copy_to_host(); @@ -72,8 +72,8 @@ int slid_over_a_walk_inside_the_warps(const Target &target) { for (int x = 0; x < W; x++) { float correct = (float)N * (float)(y + 1) * (float)(x + 1); if (result(x, y) != correct) { - printf("result(%d, %d) = %f instead of %f\n", - x, y, (double)result(x, y), (double)correct); + printf("result(%d, %d) = %f instead of %f (memory type %d)\n", + x, y, (double)result(x, y), (double)correct, (int)mem); return 1; } } @@ -186,8 +186,12 @@ int main(int argc, char **argv) { "accepted is not exercised.\n"); return 0; } - if (slid_over_a_walk_inside_the_warps(target) != 0) { - return 1; + // Both memory types are private to a thread, and check_gpu_cross_talk + // checks both, so the schedule is allowed for both. + for (MemoryType mem : {MemoryType::Register, MemoryType::Stack}) { + if (slid_over_a_walk_inside_the_warps(target, mem) != 0) { + return 1; + } } printf("Success!\n"); From c392c095c647af2d3756d912c6afb4838bd686ae Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Wed, 26 Aug 2026 11:43:33 -0700 Subject: [PATCH 7/8] Make the warps in the register-storage test actually share the work The block covered one warp's worth of rows, and the accumulator inside it split that by a warp's worth again, leaving one thread to walk all of them while the other fifteen sat in an if. The schedule the test is here to exercise has several warps sharing a walk, so give the block all of their rows. The accesses it produces now mention the thread, where before they did not, which is what a warp owning its own stripe looks like. Co-Authored-By: Claude Opus 5 --- .../gpu_register_stored_outside_thread_loop.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index baa375f62402..84b4fdf1b2e0 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -23,10 +23,10 @@ using namespace Halide; namespace { -// The width of the accumulator, how many rows there are, how many warps share -// them, and how many steps the walk takes. -const int W = 8, Y = 64, WARPS = 4, N = 16; -const int rows_per_warp = Y / WARPS; +// The width of the accumulator, how many rows a warp takes, how many warps +// share a block, how many blocks there are, and how many steps the walk takes. +const int W = 8, rows_per_warp = 16, WARPS = 4, BLOCKS = 2, N = 16; +const int Y = rows_per_warp * WARPS * BLOCKS; // A producer stored above a serial walk, computed within the loop over warps // inside it, and slid over the walk. Each warp writes and reads only its own @@ -45,7 +45,12 @@ int slid_over_a_walk_inside_the_warps(const Target &target, MemoryType mem) { out(x, y) = acc(x, y); out.bound(x, 0, W).bound(y, 0, Y).compute_root(); - out.split(y, yo, yw, rows_per_warp).gpu_blocks(yo).gpu_threads(yw); + // A block holds the warps that share the walk, and each of them a stripe + // of rows, so that the loop over warps below has all of them in it. + out.split(y, yo, yw, rows_per_warp * WARPS) + .split(yw, yw, yi, rows_per_warp) + .gpu_blocks(yo) + .gpu_threads(yw); acc.compute_at(out, yo) .split(y, yw, yi, rows_per_warp) From 20218defc2644a07df89fecdf5e29555f9e45429 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Wed, 26 Aug 2026 11:48:00 -0700 Subject: [PATCH 8/8] Unroll what indexes the register storage in the 9376 test Shrink-wrapping block-level register allocations (#9343) gives each distinct access its own registers, which it can only do when it knows which access is which. The schedule here left three loops indexing the producer: the rows a warp owns, on both the writing and the reading side, and the two live steps it rotates between. Unroll all three and the schedule this PR is about compiles against it. That is the right answer rather than a workaround. A store whose target depends on a loop variable is not a register, and the rejected pair here was one that coincides on exactly one iteration of that loop and is disjoint on the rest. Co-Authored-By: Claude Opus 5 --- .../gpu_register_stored_outside_thread_loop.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/test/correctness/gpu_register_stored_outside_thread_loop.cpp b/test/correctness/gpu_register_stored_outside_thread_loop.cpp index 84b4fdf1b2e0..b83d626bb7f1 100644 --- a/test/correctness/gpu_register_stored_outside_thread_loop.cpp +++ b/test/correctness/gpu_register_stored_outside_thread_loop.cpp @@ -54,18 +54,28 @@ int slid_over_a_walk_inside_the_warps(const Target &target, MemoryType mem) { acc.compute_at(out, yo) .split(y, yw, yi, rows_per_warp) - .gpu_threads(yw); + .gpu_threads(yw) + .unroll(yi); // The walk is outside the loop over warps, so the warps run it together. + // The rows a warp owns are unrolled, because storage a thread indexes with + // a loop variable cannot be registers. acc.update() .split(y, yw, yi, rows_per_warp) .reorder(x, yi, yw, rt) - .gpu_threads(yw); + .gpu_threads(yw) + .unroll(yi); // Stored above the walk - and so above the loop over warps - computed // within the warps, and folded down to the two steps that are live. + // + // Everything that indexes it is unrolled, here and above, because a + // register is only a register if which one it is, is known: the rows a + // warp owns on both sides, and the two live steps it rotates between. p.store_at(out, yo) .compute_at(acc, yw) - .store_in(mem); + .store_in(mem) + .unroll(y) + .unroll(t); Buffer result = out.realize({W, Y}, target); result.copy_to_host();