Various fixes to static-dimensioned Buffer #6589

steven-johnson · 2022-01-27T00:40:15Z

More details that should have been in #6574:

Buffer needed to make some methods constexpr, and also had a broken return value for as<>()
Various templated methods needed to add int Dims as a parameter
Generator::add_input() and add_output() needed specializations for static-buffer types
sliced() and embedded() should use the default values for InClassDimStorage
halide_image_io should use a named constant

- Buffer needed to make some methods constexpr, and also had a broken return value for `as<>()` - Various templated methods needed to add `int Dims` as a parameter - Generator::add_input() and add_output() needed specializations for static-buffer types - sliced() and embedded() should use the default values for InClassDimStorage - halide_image_io should use a named constant

abadams · 2022-01-27T01:26:13Z

src/Generator.h

-             typename std::enable_if<T::has_static_halide_type>::type * = nullptr>
+             typename std::enable_if<!std::is_arithmetic<T>::value && !std::is_same<T, Halide::Func>::value>::type * = nullptr>
+    GeneratorInput<T> *add_input(const std::string &name, const Type &t, int dimensions) {
+        static_assert(!T::has_static_dimensions, "You can only call this version of add_input() for a Buffer<T, D> where T is void or omitted .");


These static assert conditions seem to be the same on both lines

whoops, first should be has_static_halide_type

abadams · 2022-01-27T01:26:22Z

src/Generator.h

+    template<typename T,
+             typename std::enable_if<!std::is_arithmetic<T>::value && !std::is_same<T, Halide::Func>::value>::type * = nullptr>
+    GeneratorInput<T> *add_input(const std::string &name) {
+        static_assert(T::has_static_dimensions, "You can only call this version of add_input() for a Buffer<T, D> where T is not void.");


Same comment here

abadams · 2022-01-27T01:26:35Z

src/Generator.h

    GeneratorOutput<T> *add_output(const std::string &name, const Type &t, int dimensions) {
+        static_assert(!T::has_static_dimensions, "You can only call this version of add_output() for a Buffer<T, D> where T is void or omitted .");


abadams · 2022-01-27T01:26:47Z

src/Generator.h

+    template<typename T,
+             typename std::enable_if<!std::is_arithmetic<T>::value && !std::is_same<T, Halide::Func>::value>::type * = nullptr>
+    GeneratorOutput<T> *add_output(const std::string &name) {
+        static_assert(T::has_static_dimensions, "You can only call this version of add_output() for a Buffer<T, D> where T is not void.");


and here, unless I'm really not understanding something

abadams · 2022-01-27T01:27:51Z

tools/halide_image_io.h

@@ -2088,31 +2090,32 @@ struct ImageTypeConversion {
        // as documentation and a backstop against breakage.
        static_assert(!ImageType::has_static_halide_type,
                      "This variant of convert_image() requires a dynamically-typed image");
+        constexpr int DimsUnconstrained = Internal::DimsUnconstrained;


Would "AnyDims" be a better name than "DimsUnconstrained"?

Unconstrained is the term we use in HalideBuffer.h so that's what I used here.

I know, I was reconsidering the name in general. Seeing it in usage makes it seem clunky.

Ah, gotcha. I think I originally called it "DynamicDims" and @zvookin suggested "DimsUnconstrained". "AnyDims" is fine with me too and definitely terser. I'll make the change everywhere if you like.

steven-johnson · 2022-01-27T01:56:46Z

PTAL

abadams · 2022-01-27T17:30:54Z

src/Func.h

@@ -2615,7 +2615,7 @@ HALIDE_NO_USER_CODE_INLINE T evaluate_may_gpu(const Expr &e) {
    Func f;
    f() = e;
    Internal::schedule_scalar(f);
-    Buffer<T> im = f.realize();
+    Buffer<T, 0> im = f.realize();


This is just a minor stack space optimization, right? I don't see that this zero makes much of a difference because the dims array is never accessed. I have no objection to adding it, I just want to make sure I understand what's happening.

Correct, it's just a tiny optimization.

(On that note: the code for Buffer currently always allocates space for at least one halide_dimension_t, even for zero-dim buffers, on the assumption that there is probably sloppy code out there that assumes that buffer.dim always points to something valid...)

* Various fixes to static-dimensioned Buffer - Buffer needed to make some methods constexpr, and also had a broken return value for `as<>()` - Various templated methods needed to add `int Dims` as a parameter - Generator::add_input() and add_output() needed specializations for static-buffer types - sliced() and embedded() should use the default values for InClassDimStorage - halide_image_io should use a named constant * Update Generator.h

steven-johnson requested a review from abadams January 27, 2022 00:40

abadams reviewed Jan 27, 2022

View reviewed changes

Update Generator.h

79dcb45

abadams reviewed Jan 27, 2022

View reviewed changes

abadams approved these changes Jan 27, 2022

View reviewed changes

steven-johnson merged commit d27866e into master Jan 27, 2022

steven-johnson deleted the srj/static-buffer-fixes branch January 27, 2022 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes to static-dimensioned Buffer #6589

Various fixes to static-dimensioned Buffer #6589

steven-johnson commented Jan 27, 2022 •

edited

Loading

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

steven-johnson commented Jan 27, 2022

abadams Jan 27, 2022

steven-johnson Jan 27, 2022

		GeneratorOutput<T> *add_output(const std::string &name, const Type &t, int dimensions) {
		static_assert(!T::has_static_dimensions, "You can only call this version of add_output() for a Buffer<T, D> where T is void or omitted .");

Various fixes to static-dimensioned Buffer #6589

Various fixes to static-dimensioned Buffer #6589

Conversation

steven-johnson commented Jan 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

steven-johnson commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Jan 27, 2022 •

edited

Loading