Expand optional static-typing for Buffer to include dimensionality #6574

steven-johnson · 2022-01-20T23:20:11Z

Currently, Halide::Runtime::Buffer<>* allows you to provide a static ElementType, which allows you to enforce compile-time type checking to ensure incompatible Buffers aren't intermixed. You can also defeat this by using an ElementType of void, which defers all type checking to runtime.

Unfortunately, there isn't a way to do the same for the dimensionality of a Buffer; all checking for matching dimensionality is done at runtime. This PR proposes to extend Halide::Runtime::Buffer<>* to allow for optional specification of dimensionality as well, with only a slight incompatibility with existing code.

Currently, H::R::B<> is a template with two arguments:

H::R::B<typename T = void, int D = 4>
- T, which is the element type; if omitted, defaults to void.
- D, which specifies the amount of storage to reserve for dimensionality (but not the actual dimensionality); if omitted, defaults to 4. Note that I've often seen this argument specified on the (mistaken) assumption that it is the dimensionality! In practice, almost no code ever needs to specify this (with apps/hannk being the only notable counterexample that comes to mind).

This PR proposes to change H::R::B<> to a template with three arguments:

H::R::B<typename T = void, int Dims = -1, int InClassDimStorage = ...>
- T, which is the element type; if omitted, defaults to void. This is unchanged from previous behavior.
- Dims, which specifies the dimensionality of the Buffer:
  - If Dims >= 0, the Buffer must always have dimensions() == Dims; attempts to (e.g.) construct or access such a buffer with the 'wrong' number of arguments will fail at compile time, assignment between Buffers with different dimensionality will fail at compile time, slice() and embed() will return properly-typed results, etc.
  - If Dims == -1, the Buffer can have any dimensionality, with compatibility being checked at runtime. (Basically, the same behavior as before.)
- InClassDimStorage, which is the same as the old D argument. Its default value is determined like so:
  - If Dims is >= 1, its default value is Dims, since we shouldn't ever see any more or less.
  - If Dims is 0, we use a default value of 1 (just to be defensive against rogue code that assumes a valid pointer for dims in all cases)
  - Otherwise, it defaults to 4 (i.e., the same value as before)

TL;DR:

Code that uses Buffer<>, Buffer<void>, or Buffer<SomeType> should work as-is, unchanged, with this change.
Code that uses Buffer<void, SomeInt> or Buffer<void, SomeInt> will need attention to work after this change, but most uses I've seen fail at compile time and are easy to spot.

Note that the Generator infrastructure was expanded to add appropriate overloads for Input<> and Output<> to infer dimensionality from the Buffer specification (and metadata_tester_generator.cpp was modified to use this approach, for testing purposes); if this PR is approved, it is my intent to follow up with a PR that deprecates the ability to specify dimensionality in the old way.

*all this also applies to Halide::Buffer<> as well, just omitting it above for simplicitly

This deprecates the ctors to Generator Input/Output that are "redundant" after #6574 lands (i.e.: type and dimensions should be specified in the Buffer<T, D> declaration instead of as arguments). Also updates all our existing Generators to conform. Additive to #6574.

zvookin · 2022-01-21T03:01:30Z

src/runtime/HalideBuffer.h

 *
 * The class optionally allocates and owns memory for the image using
 * a shared pointer allocated with the provided allocator. If they are
 * null, malloc and free are used.  Any device-side allocation is
 * considered as owned if and only if the host-side allocation is
 * owned. */
-template<typename T = void, int D = 4>
+template<typename T = void,
+         int Dims = -1,


This probably ought to introduce a constant, probably declared in class, for the -1. The name is something like "DimsUnconstrained".

DynamicDims is defined (line 188) but it isn't visible to the template parameters; AFAICT I'd need to declare it outside the class. Would that be preferable?

steven-johnson · 2022-01-26T16:48:36Z

I think this is ready to land. Reviewers, please do your thing.

abadams · 2022-01-26T17:07:55Z

apps/fft/fft_aot_test.cpp

@@ -61,7 +61,7 @@ int main(int argc, char **argv) {
        auto in = real_buffer();
        for (int j = 0; j < kSize; j++) {
            for (int i = 0; i < kSize; i++) {
-                in(i, j) = signal_1d[i] + signal_1d[j];
+                in(i, j, 0) = signal_1d[i] + signal_1d[j];


Add a comment explaining why real_buffer() is not 2D (The fft requires is written generically to require 3D inputs, even when they are real).

abadams · 2022-01-26T17:13:38Z

src/runtime/HalideBuffer.h

@@ -521,7 +556,9 @@ class Buffer {
    }

    /** Get the dimensionality of the buffer. */
+    // TODO: make constexpr, optimize for const case


Why not handle this now. I think it's enough to just have an if (has_static_dimensions)

abadams · 2022-01-26T17:15:40Z

src/runtime/HalideBuffer.h

@@ -885,6 +932,7 @@ class Buffer {
     * take ownership of the data, and does not set the host_dirty flag. */
    template<typename Array, size_t N>
    explicit Buffer(Array (&vals)[N]) {
+        // TODO: this could probably be made constexpr


IMO either handle the TODO now or remove it. If the compiler can constant-fold it, it will. There shouldn't be any benefit in explicitly marking it as constexpr.

Done (comment removed)

abadams · 2022-01-26T17:17:45Z

Updating the apps to use static dimensionality in their generator inputs is left for a future PR? If you don't intend to do it right away, perhaps open a tracking issue.

steven-johnson · 2022-01-26T17:21:00Z

Updating the apps to use static dimensionality in their generator inputs is left for a future PR? If you don't intend to do it right away, perhaps open a tracking issue.

#6584

A lot of that is done in #6575; it would be easy enough to pull out those changes (without the deprecation part) and merge them.

abadams · 2022-01-26T18:11:31Z

Oh right, I forgot about that other PR.

- Allow as<> to optionally convert dimensionality as well - The default ctor shouldn't always assume Dims == 0 - make_static_shape_storage() can be more constrained

… parameter too

Convert most of the apps to use static Buffer dimensions where it makes sense. (This uncovered a few glitches in #6574.)

steven-johnson · 2022-01-26T20:03:14Z

Working on #6585 found a few oversights I missed, PTAL if you like

* wip * Update metadata_tester_generator.cpp * Update Generator.h * Update Generator.h * DynamicDims -> BufferDimsUnconstrained * Update fft_aot_test.cpp * Update HalideBuffer.h * Update HalideBuffer.h * Fix as<>(), default ctor in HalideBuffer.h - Allow as<> to optionally convert dimensionality as well - The default ctor shouldn't always assume Dims == 0 - make_static_shape_storage() can be more constrained * Add extra template arg to Buffer<>::as<> also * Realization::operator Buffer() needs to know about the extra template parameter too * halide_image_io.h needs some attention * Convert apps/ to use static Buffer dims where useful Convert most of the apps to use static Buffer dimensions where it makes sense. (This uncovered a few glitches in #6574.)

steven-johnson added 2 commits January 20, 2022 14:57

wip

279ecd6

Update metadata_tester_generator.cpp

c016a10

steven-johnson added enhancement New user-visible features or improvements to existing features. release_notes For changes that may warrant a note in README for official releases. labels Jan 20, 2022

steven-johnson added 2 commits January 20, 2022 15:31

Update Generator.h

1e7a5ef

Update Generator.h

67e75af

steven-johnson mentioned this pull request Jan 21, 2022

Deprecate GeneratorInput/Output ctors that are obsolete after #6574 #6575

Closed

zvookin reviewed Jan 21, 2022

View reviewed changes

steven-johnson added 2 commits January 21, 2022 14:48

DynamicDims -> BufferDimsUnconstrained

954e740

Merge branch 'master' into srj/static-buffer-dimensions

b2fe5cc

steven-johnson requested review from abadams and zvookin January 25, 2022 22:42

abadams reviewed Jan 26, 2022

View reviewed changes

abadams approved these changes Jan 26, 2022

View reviewed changes

steven-johnson mentioned this pull request Jan 26, 2022

Update apps, tutorials etc to use static-dimensionality Buffers, where appropriate #6584

Closed

steven-johnson added 3 commits January 26, 2022 09:23

Update fft_aot_test.cpp

9e12a9e

Update HalideBuffer.h

55866e4

Update HalideBuffer.h

02ff5d9

steven-johnson added 4 commits January 26, 2022 11:58

Fix as<>(), default ctor in HalideBuffer.h

e5d8cd8

- Allow as<> to optionally convert dimensionality as well - The default ctor shouldn't always assume Dims == 0 - make_static_shape_storage() can be more constrained

Add extra template arg to Buffer<>::as<> also

beda5a9

Realization::operator Buffer() needs to know about the extra template…

67f57ab

… parameter too

halide_image_io.h needs some attention

63f8094

steven-johnson added a commit that referenced this pull request Jan 26, 2022

Convert apps/ to use static Buffer dims where useful

d58a09d

Convert most of the apps to use static Buffer dimensions where it makes sense. (This uncovered a few glitches in #6574.)

steven-johnson mentioned this pull request Jan 26, 2022

Convert apps/ to use static Buffer dims where useful #6585

Merged

steven-johnson merged commit 892b558 into master Jan 26, 2022

steven-johnson deleted the srj/static-buffer-dimensions branch January 26, 2022 21:42

steven-johnson mentioned this pull request Jan 27, 2022

Various fixes to static-dimensioned Buffer #6589

Merged

AtilaSaraiva mentioned this pull request Jul 3, 2022

halide: 10.0.0 -> 14.0.0 NixOS/nixpkgs#180015

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand optional static-typing for Buffer to include dimensionality #6574

Expand optional static-typing for Buffer to include dimensionality #6574

steven-johnson commented Jan 20, 2022 •

edited by alexreinking

Loading

zvookin Jan 21, 2022

steven-johnson Jan 21, 2022

steven-johnson Jan 21, 2022

steven-johnson commented Jan 26, 2022

abadams Jan 26, 2022

steven-johnson Jan 26, 2022

abadams Jan 26, 2022

steven-johnson Jan 26, 2022

abadams Jan 26, 2022

steven-johnson Jan 26, 2022

abadams commented Jan 26, 2022

steven-johnson commented Jan 26, 2022

abadams commented Jan 26, 2022

steven-johnson commented Jan 26, 2022

Expand optional static-typing for Buffer to include dimensionality #6574

Expand optional static-typing for Buffer to include dimensionality #6574

Conversation

steven-johnson commented Jan 20, 2022 • edited by alexreinking Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Jan 26, 2022

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

abadams commented Jan 26, 2022

steven-johnson commented Jan 26, 2022

abadams commented Jan 26, 2022

steven-johnson commented Jan 26, 2022

steven-johnson commented Jan 20, 2022 •

edited by alexreinking

Loading