From 2c01cc2e45a51202163c3349ef28e02488eac051 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Thu, 16 Jul 2026 12:59:44 -0700 Subject: [PATCH 1/5] Assert callable arg count matches buffer dimensionality in for_each_element Buffer::for_each_element previously accepted a callable taking fewer int arguments than the buffer's dimensionality, silently iterating only over a reduced-dimensionality slice. A nullary callable (e.g. buffer.fill(rng)) therefore wrote only the origin element. This is an easy mistake to make and produces no error. Require the callable to take either a const int *, or exactly as many int arguments as the buffer has dimensions. This uncovered three real bugs where buffers were only ever having their first element randomized: - test/fuzz/lossless_cast.cpp: buf.fill(fuzz) - test/correctness/interleave_rgb.cpp: fill([]() { return rand(); }) - test/correctness/multiple_scatter.cpp: fill(std::mt19937{0}) All three are fixed to use for_each_value, which passes an element reference and does not depend on coordinates. The reduced-slice usage in test/correctness/for_each_element.cpp is rewritten to slice off a channel first so the callable's arg count matches the iteration domain. Co-Authored-By: Claude Opus 4.8 --- src/runtime/HalideBuffer.h | 20 +++++++++----------- test/correctness/for_each_element.cpp | 6 ++++-- test/correctness/interleave_rgb.cpp | 2 +- test/correctness/multiple_scatter.cpp | 3 ++- test/fuzz/lossless_cast.cpp | 4 ++-- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/runtime/HalideBuffer.h b/src/runtime/HalideBuffer.h index b49272d0f962..219fad464cb2 100644 --- a/src/runtime/HalideBuffer.h +++ b/src/runtime/HalideBuffer.h @@ -2463,7 +2463,10 @@ class Buffer { template HALIDE_ALWAYS_INLINE static void for_each_element(double, int dims, const for_each_element_task_dim *t, Fn &&f) { int args = num_args(0, std::forward(f)); - assert(dims >= args); + assert(args == dims && + "Callable passed to for_each_element (or fill) must accept either a " + "const int *, or a number of int arguments equal to the dimensionality " + "of the buffer."); for_each_element_variadic(0, args - 1, t, std::forward(f)); } @@ -2481,19 +2484,14 @@ class Buffer { public: /** Call a function at each site in a buffer. This is likely to be * much slower than using Halide code to populate a buffer, but is - * convenient for tests. If the function has more arguments than the - * buffer has dimensions, the remaining arguments will be zero. If it - * has fewer arguments than the buffer has dimensions then the last - * few dimensions of the buffer are not iterated over. For example, - * the following code exploits this to set a floating point RGB image - * to red: + * convenient for tests. The function must take a number of int + * arguments equal to the dimensionality of the buffer. For example, + * the following code sets a floating point RGB image to red: \code Buffer im(100, 100, 3); - im.for_each_element([&](int x, int y) { - im(x, y, 0) = 1.0f; - im(x, y, 1) = 0.0f; - im(x, y, 2) = 0.0f: + im.for_each_element([&](int x, int y, int c) { + im(x, y, c) = (c == 0) ? 1.0f : 0.0f; }); \endcode diff --git a/test/correctness/for_each_element.cpp b/test/correctness/for_each_element.cpp index 85fcfc2380fd..487d1e0e16b8 100644 --- a/test/correctness/for_each_element.cpp +++ b/test/correctness/for_each_element.cpp @@ -27,8 +27,10 @@ int main(int argc, char **argv) { im(pos) *= 3; }); - im.for_each_element([&](int x, int y) { - for (int c = 0; c < 3; c++) { + // Slice off a channel to get a two-dimensional domain to iterate + // over, then handle all channels inside the callable. + im.sliced(2, im.dim(2).min()).for_each_element([&](int x, int y) { + for (int c : im.dim(2)) { int correct = (10 * x + 5 * y + c) * 3; if (im(x, y, c) != correct) { printf("im(%d, %d, %d) = %d instead of %d\n", diff --git a/test/correctness/interleave_rgb.cpp b/test/correctness/interleave_rgb.cpp index 092b54eb569a..dc50843eb9b7 100644 --- a/test/correctness/interleave_rgb.cpp +++ b/test/correctness/interleave_rgb.cpp @@ -81,7 +81,7 @@ bool test_deinterleave(int x_stride) { deinterleaved.vectorize(x, target.natural_vector_size(), TailStrategy::GuardWithIf).unroll(c); } Buffer input_buf = Buffer::make_interleaved(255, 128, x_stride); - input_buf.fill([]() { return rand(); }); + input_buf.for_each_value([](T &v) { v = rand(); }); input_buf.crop(2, 0, 3); input.set(input_buf); diff --git a/test/correctness/multiple_scatter.cpp b/test/correctness/multiple_scatter.cpp index 9c5c0387fbab..39e03d86992a 100644 --- a/test/correctness/multiple_scatter.cpp +++ b/test/correctness/multiple_scatter.cpp @@ -33,7 +33,8 @@ int main(int argc, char **argv) { Buffer input(128, 8); - input.fill(std::mt19937{0}); + std::mt19937 rng{0}; + input.for_each_value([&](int &v) { v = rng(); }); Func sorted1; Var x, y; diff --git a/test/fuzz/lossless_cast.cpp b/test/fuzz/lossless_cast.cpp index 01ef1c38e8d7..b820f97f2071 100644 --- a/test/fuzz/lossless_cast.cpp +++ b/test/fuzz/lossless_cast.cpp @@ -264,8 +264,8 @@ bool might_have_ub(Expr e) { } // namespace FUZZ_TEST(lossless_cast, FuzzingContext &fuzz) { - buf_u8.fill(fuzz); - buf_i8.fill(fuzz); + buf_u8.for_each_value([&](uint8_t &v) { v = (uint8_t)fuzz(); }); + buf_i8.for_each_value([&](int8_t &v) { v = (int8_t)fuzz(); }); Expr e1 = random_expr(fuzz); Expr simplified = simplify(e1); From e008abc66db656098c604c55387147532b91dcd0 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Thu, 16 Jul 2026 13:15:41 -0700 Subject: [PATCH 2/5] Make Buffer::fill dispatch zero-arg callables to for_each_value A zero-argument callable passed to fill (for example a random number generator) has no coordinate to consume, so there is no distinction between filling via for_each_element and for_each_value. Dispatch such callables to for_each_value, which evaluates the callable once per element. This makes the natural buffer.fill(rng) idiom fill the whole buffer instead of tripping the for_each_element arg-count assertion. With this, the previously-broken buffer initializations in interleave_rgb, multiple_scatter, and lossless_cast (which only ever randomized the origin element) are fixed by reverting them to their original fill(rng) form. Co-Authored-By: Claude Opus 4.8 --- src/runtime/HalideBuffer.h | 17 +++++++++++++---- test/correctness/interleave_rgb.cpp | 2 +- test/correctness/multiple_scatter.cpp | 3 +-- test/fuzz/lossless_cast.cpp | 4 ++-- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/runtime/HalideBuffer.h b/src/runtime/HalideBuffer.h index 219fad464cb2..de8c63fabf9a 100644 --- a/src/runtime/HalideBuffer.h +++ b/src/runtime/HalideBuffer.h @@ -2570,13 +2570,22 @@ class Buffer { /** Fill a buffer by evaluating a callable at every site. The * callable should look much like a callable passed to * for_each_element, but it should return the value that should be - * stored to the coordinate corresponding to the arguments. */ + * stored to the coordinate corresponding to the arguments. A callable + * that takes no arguments (for example a random number generator) is + * evaluated once per element, ignoring the coordinate. */ template>>> Buffer &fill(Fn &&f) { - // We'll go via for_each_element. We need a variadic wrapper lambda. - FillHelper wrapper(std::forward(f), this); - return for_each_element(wrapper); + if constexpr (std::is_invocable_v) { + // A callable that takes no coordinates has nothing to + // distinguish sites, so fill every value with its result. + for_each_value([&](T &v) { v = f(); }); + } else { + // We'll go via for_each_element. We need a variadic wrapper lambda. + FillHelper wrapper(std::forward(f), this); + for_each_element(wrapper); + } + return *this; } /** Check if an input buffer passed extern stage is a querying diff --git a/test/correctness/interleave_rgb.cpp b/test/correctness/interleave_rgb.cpp index dc50843eb9b7..092b54eb569a 100644 --- a/test/correctness/interleave_rgb.cpp +++ b/test/correctness/interleave_rgb.cpp @@ -81,7 +81,7 @@ bool test_deinterleave(int x_stride) { deinterleaved.vectorize(x, target.natural_vector_size(), TailStrategy::GuardWithIf).unroll(c); } Buffer input_buf = Buffer::make_interleaved(255, 128, x_stride); - input_buf.for_each_value([](T &v) { v = rand(); }); + input_buf.fill([]() { return rand(); }); input_buf.crop(2, 0, 3); input.set(input_buf); diff --git a/test/correctness/multiple_scatter.cpp b/test/correctness/multiple_scatter.cpp index 39e03d86992a..9c5c0387fbab 100644 --- a/test/correctness/multiple_scatter.cpp +++ b/test/correctness/multiple_scatter.cpp @@ -33,8 +33,7 @@ int main(int argc, char **argv) { Buffer input(128, 8); - std::mt19937 rng{0}; - input.for_each_value([&](int &v) { v = rng(); }); + input.fill(std::mt19937{0}); Func sorted1; Var x, y; diff --git a/test/fuzz/lossless_cast.cpp b/test/fuzz/lossless_cast.cpp index b820f97f2071..01ef1c38e8d7 100644 --- a/test/fuzz/lossless_cast.cpp +++ b/test/fuzz/lossless_cast.cpp @@ -264,8 +264,8 @@ bool might_have_ub(Expr e) { } // namespace FUZZ_TEST(lossless_cast, FuzzingContext &fuzz) { - buf_u8.for_each_value([&](uint8_t &v) { v = (uint8_t)fuzz(); }); - buf_i8.for_each_value([&](int8_t &v) { v = (int8_t)fuzz(); }); + buf_u8.fill(fuzz); + buf_i8.fill(fuzz); Expr e1 = random_expr(fuzz); Expr simplified = simplify(e1); From 51c17ba6e1e9ea8533750a026ce776201785a4ac Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Thu, 16 Jul 2026 13:28:40 -0700 Subject: [PATCH 3/5] Fix infinite recursion in FindIntrinsics::strip_widening_cast strip_widening_cast is meant to return a narrower-or-equal-width equivalent of its argument. When the argument was a narrowing cast (for example int16(int32_expr)), its innermost type was wider than the argument's type, and the consider(inner_t) branch returned an expression wider than the input. Feeding that back into the widening_mul narrowing rewrite re-widened the multiply, so the pass ping-ponged a widening_mul between its int32 and int64 forms forever, overflowing the stack. Only consider the inner type when it is no wider than the argument, i.e. when there is a genuine widening cast to strip. Found by the lossless_cast fuzzer once buffer.fill was fixed to actually randomize its input buffers. Adds a deterministic regression test. Co-Authored-By: Claude Opus 4.8 --- src/FindIntrinsics.cpp | 8 +++++- test/correctness/lossless_cast.cpp | 39 ++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/FindIntrinsics.cpp b/src/FindIntrinsics.cpp index 87cb325c3fc6..612f3063d7fb 100644 --- a/src/FindIntrinsics.cpp +++ b/src/FindIntrinsics.cpp @@ -122,7 +122,13 @@ class FindIntrinsics : public IRMutator { } }; - if (inner_t.is_int_or_uint() && inner_t != x.type()) { + // Only strip a genuine widening cast, i.e. one whose inner type is no + // wider than x. If the outer cast is a narrowing cast, inner_t is wider + // than x, and considering it would return an expression wider than x, + // which can send the surrounding narrowing rewrites into infinite + // recursion. + if (inner_t.is_int_or_uint() && inner_t != x.type() && + inner_t.bits() <= x.type().bits()) { consider(inner_t); } consider(x.type().narrow()); diff --git a/test/correctness/lossless_cast.cpp b/test/correctness/lossless_cast.cpp index 8959b72ccd53..e9d3d99055d0 100644 --- a/test/correctness/lossless_cast.cpp +++ b/test/correctness/lossless_cast.cpp @@ -274,6 +274,45 @@ int main(int argc, char **argv) { } } + // Runtime test: a widening_mul whose operand is a narrowing cast + // (int32 -> int16) of a widening_add used to send FindIntrinsics into + // infinite recursion. strip_widening_cast could return an expression wider + // than its input, which the widening_mul narrowing rewrite would re-widen, + // ping-ponging the multiply between its int32 and int64 forms forever. This + // must simply compile, and the vectorized result must match a scalar + // reference. + { + Var x("x"); + Buffer buf(64, "buf"); + for (int i = 0; i < 64; i++) { + buf(i) = (int8_t)(i - 32); + } + + Expr t = buf(x); + Expr inner = cast(Int(16), cast(UInt(8), t) + cast(UInt(8), 68)); + Expr a = widen_right_add(inner, t); + Expr b = widen_right_mul(a, cast(Int(8), a) * cast(Int(8), 39)); + Expr wide_add = widening_add(a, b); + Expr e = cast(Int(64), widening_mul(cast(Int(16), 73), cast(Int(16), wide_add))); + + Func f_vec, f_ref; + f_vec(x) = e; + f_ref(x) = e; + f_vec.vectorize(x, 4, TailStrategy::RoundUp); + + Buffer out_vec = f_vec.realize({64}); + Buffer out_ref = f_ref.realize({64}); + + for (int i = 0; i < 64; i++) { + if (out_vec(i) != out_ref(i)) { + std::cerr << "Recursion regression test: mismatch at x=" << i + << ": ref=" << out_ref(i) << " vectorized=" << out_vec(i) << "\n"; + found_error = true; + break; + } + } + } + if (found_error) { return 1; } From c42488caf7367079ce1e176ee11d405aef0e5e69 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Thu, 16 Jul 2026 15:44:31 -0700 Subject: [PATCH 4/5] Fix rgb_interleaved performance test for stricter for_each_element The for_each_element arg-count assertion rejects a callable taking fewer int arguments than the buffer's dimensionality. These calls iterated a 2D domain over a 3D buffer and handled all channels inside the callable, so slice off the channel dimension first to get a matching 2D domain. Co-Authored-By: Claude Opus 4.8 --- test/performance/rgb_interleaved.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/performance/rgb_interleaved.cpp b/test/performance/rgb_interleaved.cpp index 3adfbeda4f52..68b53fc81d86 100644 --- a/test/performance/rgb_interleaved.cpp +++ b/test/performance/rgb_interleaved.cpp @@ -33,7 +33,7 @@ void test_deinterleave() { // Setup dst to be planar, with no extra padding between channels or rows. Buffer dst_image(1 << 12, 1 << 12, 3); - src_image.for_each_element([&](int x, int y) { + src_image.sliced(2, 0).for_each_element([&](int x, int y) { src_image(x, y, 0) = 0; src_image(x, y, 1) = 128; src_image(x, y, 2) = 255; @@ -54,7 +54,7 @@ void test_deinterleave() { printf("Interleaved to planar bandwidth %.3e byte/s.\n", dst_image.number_of_elements() / t1); - dst_image.for_each_element([&](int x, int y) { + dst_image.sliced(2, 0).for_each_element([&](int x, int y) { assert(dst_image(x, y, 0) == 0); assert(dst_image(x, y, 1) == 128); assert(dst_image(x, y, 2) == 255); @@ -69,7 +69,7 @@ void test_deinterleave() { dst.realize(dst_image); }); - dst_image.for_each_element([&](int x, int y) { + dst_image.sliced(2, 0).for_each_element([&](int x, int y) { assert(dst_image(x, y, 0) == 0); assert(dst_image(x, y, 1) == 128); assert(dst_image(x, y, 2) == 255); @@ -111,7 +111,7 @@ void test_interleave(bool fast) { // Setup dst to be interleaved Buffer dst_image = Buffer::make_interleaved(1 << 12, 1 << 12, 3); - src_image.for_each_element([&](int x, int y) { + src_image.sliced(2, 0).for_each_element([&](int x, int y) { src_image(x, y, 0) = 0; src_image(x, y, 1) = 128; src_image(x, y, 2) = 255; @@ -136,7 +136,7 @@ void test_interleave(bool fast) { printf("Planar to interleaved bandwidth %.3e byte/s.\n", dst_image.number_of_elements() / t); - dst_image.for_each_element([&](int x, int y) { + dst_image.sliced(2, 0).for_each_element([&](int x, int y) { assert(dst_image(x, y, 0) == 0); assert(dst_image(x, y, 1) == 128); assert(dst_image(x, y, 2) == 255); From a1721ab245e017bb4a95ea5e6489e0035495e640 Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Thu, 16 Jul 2026 20:51:41 -0700 Subject: [PATCH 5/5] Mark host buffer dirty when filling with a zero-arg callable The zero-arg fill path fills via for_each_value, which writes through raw element references and does not set the host_dirty flag the way operator() does. Without it, a subsequent GPU run does not copy the freshly filled data to the device and reads stale contents. Set host_dirty explicitly, as the scalar fill overload already does. Co-Authored-By: Claude Opus 4.8 --- src/runtime/HalideBuffer.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/runtime/HalideBuffer.h b/src/runtime/HalideBuffer.h index de8c63fabf9a..f95541221c2b 100644 --- a/src/runtime/HalideBuffer.h +++ b/src/runtime/HalideBuffer.h @@ -2579,6 +2579,9 @@ class Buffer { if constexpr (std::is_invocable_v) { // A callable that takes no coordinates has nothing to // distinguish sites, so fill every value with its result. + // for_each_value writes through raw references, so it does not + // mark the host buffer dirty the way operator() does. + set_host_dirty(); for_each_value([&](T &v) { v = f(); }); } else { // We'll go via for_each_element. We need a variadic wrapper lambda.