Scheduling directive to support ring buffering #7967

vksnk · 2023-11-30T03:52:42Z

This PR adds a new directive which allows to explicitly express double buffering via schedule.

The simple motivating example is a pipeline of two functions A and B, where B depends A. One common computational pattern is to process a tile tile_index of A, start processing of the tile tile_index of B and simultaneously start processing of the tiletile_index + 1 of A, overlaping the computation for A and B in time. It's currently possible to express some subset of such schedules using fold_storage and async, but it's tied to the storage folding logic and it's not possible to double buffer at arbitrary loop level.

The new double_buffer directive can be used together with hoist_storage to express the pipeline above like this:

Func A("A"), B("B");
Var x, y, xo, yo, xi, yi;

A(x, y) = x + y;
B(x, y) = A(x - 1, y - 1) + A(x, y) + A(x + 1, y + 1);

B.compute_root()
    .tile(x, y, xo, yo, xi, yi, 16, 16, TailStrategy::RoundUp);
A.compute_at(B, xo)
    .hoist_storage_root()
    .double_buffer()
    .async();

At the high-level, the schedule will move the storage of A outside of the loops over tiles (in this case there are two loops over tiles xo and yo), expand it by 2x by adding a new dimension to corresponding Realize node, and add corresponding syncronization (similar to what fold_storage would do) to make sure it's safe with async and A can run ahead by one tile.

This reverts commit eb9dd02.

steven-johnson · 2023-11-30T15:00:26Z

This is definitely gonna need a tutorial :-)

…#7971) All known AVX2-enabled architectures definitely have these features.

mcourteaux

Amazing update! A few minor comments: mainly about traces of "double buffer" naming not being replaced by "ring_buffer".

src/AsyncProducers.cpp

test/error/CMakeLists.txt

test/error/bad_double_buffer.cpp

test/correctness/CMakeLists.txt

src/Schedule.h

src/Func.h

steven-johnson

Absolutely beautiful work, comments are mostly nits, except:

We should add Python bindings (and at least a basic test using them) before landing this, but that's trivial (and I can contribute it directly to this PR if you like, just LMK)

src/AsyncProducers.cpp

steven-johnson · 2023-12-12T17:08:26Z

src/AsyncProducers.cpp

+        auto it = env.find(op->name);
+        internal_assert(it != env.end());
+        Function f = it->second;
+        if (f.schedule().async() && hoisted_storages.count(op->name) == 0) {


nit: some clang-tidy variants will always want to prefer .empty() to count() == .

hoisted_storage is std::set and the count() is basically used to see if the elements is in the set. I am not sure how empty can be used instead.

src/AsyncProducers.cpp

steven-johnson · 2023-12-12T17:35:52Z

test/correctness/double_buffer.cpp

+        consumer
+            .compute_root()
+            .tile(x, y, xo, yo, xi, yi, 16, 16, TailStrategy::RoundUp);
+        producer


Suggestion: even though this is just a test, it's worth adding some comments about what this schedule is doing and how the storage schedules interact.

I've made test descriptions a bit more concrete.

steven-johnson · 2023-12-12T17:36:24Z

test/correctness/double_buffer.cpp

+        });
+    }
+
+    // Two non-async double-buffered producers and two consumers.


Have we tested this under TSAN yet?

tutorial/lesson_24_async.cpp

steven-johnson · 2023-12-12T17:43:54Z

tutorial/lesson_24_async.cpp

+        consumer.compute_root()
+            .tile(x, y, xo, yo, xi, yi, 16, 16, TailStrategy::Auto);
+
+        producer


Suggestion: It would be useful to show what the storage and execution-time of each schedule is.

I've added a couple of notes that storage is going to be doubled across the tutorial.

steven-johnson · 2023-12-12T17:45:12Z

tutorial/lesson_24_async.cpp

+        consumer.realize({128, 128, 4});
+    }
+
+    {


I'm a little confused about why the previous example exists -- it seems like we're trying to show that you can use fold_storage for this purpose, but IIUC, it's an awkward tool for the job and ring_buffer is really a better choice? If so, are we doing the user a service by giving them a complex example of how not to do it?

So, the short version is for two reasons:

fold_storage() only works over storage dimensions of the Func. Not arbitrary loop levels. ring_buffer() can be used over any loop (not only those that explicitly go over a storage dimension). In general, I feel like this is an awkward constraint.

ring_buffer() not only ring buffers, but also injects the semaphore logic to make sure that no race condition happens when combining it with async().

I do wonder if there is any use for ring_buffer() without async(), @vksnk?

A more intuitive / clean way I can think of this mechanic is to have fold_storage() supports doing it over any loop level, and to have async() insert the semaphore synchronization over folded storage. But that would totally break the current implementation of this PR.

he previous example exists -- it seems like we're trying to show that you can use fold_storage for this purpose, but IIUC, it's an awkward tool for the job and ring_buffer is really a better choice? If so, are we doing the user a service by giving them a complex example of how not to do it?

That's a good point, but I felt it's important to have it for completeness. Also, this construct can be useful if you actually want an actual sliding window with async.

I do wonder if there is any use for ring_buffer() without async(), @vksnk?

It's not super useful right now, but it might be useful for some backends. I'm planning to use it for one of the DSPs, which has an async API for DMA (but doesn't really support threads that well).

A more intuitive / clean way I can think of this mechanic is to have fold_storage() supports doing it over any loop level, and to have async() insert the semaphore synchronization over folded storage. But that would totally break the current implementation of this PR.

I totally agree, it's slightly annoying that fold_storage and the new directive overlap a lot. Actually, one of the initial ideas was to special case fold_storage to allow something like fold_storage(Var::outermost(), extent), which still sort of fits into existing fold_storage semantics (you can argue that whenever you do hoist_storage, you actually add a new outer dimension to storage with extent 1 by default and that would be a way to control its extent). It felt not very user-friendly, so it was decided to go with a separate directive.

I totally agree, it's slightly annoying that fold_storage and the new directive overlap a lot. Actually, one of the initial ideas was to special case fold_storage to allow something like fold_storage(Var::outermost(), extent), which still sort of fits into existing fold_storage semantics (you can argue that whenever you do hoist_storage, you actually add a new outer dimension to storage with extent 1 by default and that would be a way to control its extent).

Ah, then I don't understand Var::outermost() I think. I thought it referred to the place before the outermost storage dimension, but you seem to suggest that it is actually over the outermost loop that actually exists in the schedule for this Func. Meaning that if there is a compute_at(), that removes a bunch of outermost loops, it will actually represent a loop level just outside the outermost one that exists, due to the compute_at().

It felt not very user-friendly, so it was decided to go with a separate directive.

That might have been a good use case for syntactic sugar perhaps?

Func &Func::ring_buffer(const Expr &extent) { assert(some_compute_at_is_specified); fold_storage(Var::outermost(), extent); }

As long as we okay with Func &Func::ring_buffer(const Expr &extent); API, I think we can think through it really thoroughly and if possible reconcile with fold_storage later.

…into vksnk/roll-buffer

…r are computed

vksnk · 2023-12-14T23:47:31Z

Thanks a lot for the review! I think I addressed all of the comments, please take another look.

steven-johnson · 2023-12-15T18:18:04Z

src/AsyncProducers.cpp

+            // and corresponding Realize node.
+            int loop_index = hoist_storage_loop_index[op->name] + 1;
+            Expr current_index = Variable::make(Int(32), loops[loop_index].name);
+            while (++loop_index < (int)loops.size()) {


dumb q: have we tested this on buffers with nonzero mins to be sure it is all good?

I didn't test it, but this only affect intermediate functions and their allocations, so I think buffers with nonzero mins should be ok.

steven-johnson · 2023-12-15T18:18:37Z

src/AsyncProducers.cpp

+                                    (loops[loop_index].extent - loops[loop_index].min) +
+                                Variable::make(Int(32), loops[loop_index].name);
+            }
+            current_index = current_index % f.schedule().ring_buffer();


Is this guaranteed to be > 0?

This is guaranteed to be >= 0 which us what we want.

expr % 0 will cause a divide by zero error, which is probably not what we want.

This is an Expr, so % 0 will give 0.

src/Func.h

vksnk · 2023-12-19T22:13:49Z

All comments are resolved, going to merge in.

* Half-plumbed * Revert "Half-plumbed" This reverts commit eb9dd02. * Interface for double buffer * Update Provides, Calls and Realizes for double buffering * Proper sync for double buffering * Use proper name for the semaphor and use correct initial value * Rename the class * Pass expression for index * Adds storage for double buffering index * Use a separate index to go through the double buffer * Failing test * Better handling of hoisted storage in all of the async-related passes * New test and clean-up the generated IR * More tests * Allow double buffering without async and add corresponding test * Filter out incorrect double_buffer schedules * Add tests to the cmake files * Clean up * Update the comment * Clean up * Clean up * Update serialization * complete_x86_target() should enable F16C and FMA when AVX2 is present (halide#7971) All known AVX2-enabled architectures definitely have these features. * Add two new tail strategies for update definitions (halide#7949) * Add two new tail strategies for update definitions * Stop printing asm * Update expected number of partitions for Partition::Always * Add a comment explaining why the blend safety check is per dimension * Add serialization support for the new tail strategies * trigger buildbots * Add comment --------- Co-authored-by: Steven Johnson <[email protected]> * Add appropriate mattrs for arm-32 extensions (halide#7978) * Add appropriate mattrs for arm-32 extensions Fixes halide#7976 * Pull clauses out of if * Move canonical version numbers into source, not build system (halide#7980) (halide#7981) * Move canonical version numbers into source, not build system (halide#7980) * Fixes * Silence useless "Insufficient parallelism" autoscheduler warning (halide#7990) * Add a notebook with a visualization of the aprrox_* functions and their errors (halide#7974) * Add a notebook with a visualization of the aprrox_* functions and their errors * Fix spelling error * Make narrowing float->int casts on wasm go via wider ints (halide#7973) Fixes halide#7972 * Fix handling of assert statements whose conditions get vectorized (halide#7989) * Fix handling of assert statements whose conditions get vectorized * Fix test name * Fix all "unscheduled update()" warnings in our code (halide#7991) * Fix all "unscheduled update()" warnings in our code And also fix the Mullapudi scheduler to explicitly touch all update stages. This allows us to mark this warning as an error if we so choose. * fixes * fixes * Update recursive_box_filters.cpp * Silence useless 'Outer dim vectorization of var' warning in Mullapudi… (halide#7992) Silence useless 'Outer dim vectorization of var' warning in Mullapudi scheduler * Add a tutorial for async and double_buffer * Renamed double_buffer to ring_buffer * ring_buffer() now expects an extent Expr * Actually use extent for ring_buffer() * Address some of the comments * Provide an example of the code structure for producer-consumer async example * Comments updates * Fix clang-format and clang-tidy * Add Python binding for Func::ring_buffer() * Don't use a separate index for ring buffer + add a new test * Rename the tests * Clean up the old name * Add & * Move test to the right folder * Move expr * Add comments for InjectRingBuffering * Improve ring_buffer doc * Fix comments * Comments * A better error message * Mention that extent is expected to be a positive integer * Add another code structure and explain how the indices for ring buffer are computed * Expand test comments * Fix spelling --------- Co-authored-by: Steven Johnson <[email protected]> Co-authored-by: Andrew Adams <[email protected]>

vksnk added 24 commits October 31, 2023 09:43

Half-plumbed

eb9dd02

Merge branch 'main' into vksnk/roll-buffer

b8fe0c0

Revert "Half-plumbed"

cbeb78b

This reverts commit eb9dd02.

Interface for double buffer

f3887e2

Update Provides, Calls and Realizes for double buffering

e5042a6

Merge branch 'main' into vksnk/roll-buffer

3e297e2

Proper sync for double buffering

e5cc755

Use proper name for the semaphor and use correct initial value

d521e8c

Rename the class

5d216f4

Pass expression for index

c49293f

Adds storage for double buffering index

3bc624b

Use a separate index to go through the double buffer

1a727cf

Failing test

45754f9

Better handling of hoisted storage in all of the async-related passes

e2fc6f5

New test and clean-up the generated IR

a2a838e

Merge branch 'main' into vksnk/roll-buffer

7b4b9f4

More tests

ff41546

Allow double buffering without async and add corresponding test

d083a71

Filter out incorrect double_buffer schedules

e169340

Add tests to the cmake files

f3047e7

Clean up

fbbae85

Update the comment

5d06d8d

Clean up

8d90a7f

Clean up

1479de7

vksnk requested a review from abadams November 30, 2023 03:52

vksnk added 2 commits December 1, 2023 09:12

Update serialization

c86dc8c

Merge branch 'main' into vksnk/roll-buffer

d8c6831

mcourteaux mentioned this pull request Dec 1, 2023

Inject profiling for function calls to 'halide_copy_to_host' and 'halide_copy_to_device'. #7913

Merged

complete_x86_target() should enable F16C and FMA when AVX2 is present (…

f942ccf

…#7971) All known AVX2-enabled architectures definitely have these features.

Fix clang-format and clang-tidy

97e1f40

mcourteaux requested changes Dec 12, 2023

View reviewed changes

steven-johnson changed the title ~~Scheduling directive to support double buffering~~ Scheduling directive to support ring buffering Dec 12, 2023

mcourteaux requested changes Dec 12, 2023

View reviewed changes

src/Func.h Show resolved Hide resolved

steven-johnson requested changes Dec 12, 2023

View reviewed changes

steven-johnson and others added 18 commits December 12, 2023 09:54

Merge branch 'main' into vksnk/roll-buffer

9bffeab

Add Python binding for Func::ring_buffer()

6b3314e

Don't use a separate index for ring buffer + add a new test

01b1062

Merge branch 'vksnk/roll-buffer' of https://github.com/halide/Halide …

6ec4fb6

…into vksnk/roll-buffer

Rename the tests

3ec5ef4

Clean up the old name

c5bcf9a

Add &

d189d70

Merge branch 'main' into vksnk/roll-buffer

e69443c

Move test to the right folder

f6f2192

Move expr

8200630

Add comments for InjectRingBuffering

d9d9f9f

Improve ring_buffer doc

34e15e5

Fix comments

1ef8b6f

Comments

9800b2b

A better error message

b99c01b

Mention that extent is expected to be a positive integer

d200b0f

Add another code structure and explain how the indices for ring buffe…

f000e19

…r are computed

Expand test comments

abc6e59

steven-johnson approved these changes Dec 15, 2023

View reviewed changes

Fix spelling

61d344b

vksnk merged commit 61b8d38 into main Dec 19, 2023

BrewTestBot mentioned this pull request Jul 17, 2024

halide 18.0.0 Homebrew/homebrew-core#177657

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduling directive to support ring buffering #7967

Scheduling directive to support ring buffering #7967

vksnk commented Nov 30, 2023

steven-johnson commented Nov 30, 2023

mcourteaux left a comment

steven-johnson left a comment

steven-johnson Dec 12, 2023

vksnk Dec 14, 2023

steven-johnson Dec 12, 2023

vksnk Dec 14, 2023

steven-johnson Dec 12, 2023

steven-johnson Dec 12, 2023

vksnk Dec 14, 2023

steven-johnson Dec 12, 2023

mcourteaux Dec 13, 2023

vksnk Dec 14, 2023

mcourteaux Dec 15, 2023 •

edited

Loading

vksnk Dec 19, 2023

vksnk commented Dec 14, 2023

steven-johnson Dec 15, 2023

vksnk Dec 15, 2023

steven-johnson Dec 15, 2023

vksnk Dec 15, 2023

steven-johnson Dec 15, 2023

abadams Dec 15, 2023

vksnk commented Dec 19, 2023

Scheduling directive to support ring buffering #7967

Scheduling directive to support ring buffering #7967

Conversation

vksnk commented Nov 30, 2023

steven-johnson commented Nov 30, 2023

mcourteaux left a comment

Choose a reason for hiding this comment

steven-johnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcourteaux Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vksnk commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vksnk commented Dec 19, 2023

mcourteaux Dec 15, 2023 •

edited

Loading