Fix Issue #3955

[steven-johnson]

This is a simpler alternative to #3255; it doesn't support non-Buffer classes quite as easily, but is likely to be much easier to apply to existing code.

[steven-johnson]

PTAL

[abadams]

What are the advantages of this intermediate type over just a free function that takes whatever Buffer type the user has provided?

[steven-johnson]

If we have a free function, what will the type of the argument be?

• If it's BufType&, the arg has to be an lvalue, so you can't pass temporaries.
• If it's const BufType&, then we have to do a const_cast internally, and I'm convinced that every time you do a const_cast, God kills a kitten.
• if it's just plain BufType, you may end up with unnecessary copies (if the value isn't provably movable by the compiler).

[abadams]

I tried BufType && but I can't get the return type to work. A temporary object seems to be necessary.

[steven-johnson]

There may well be a slicker way to do it, but until/unless we find someone who really needs the extra flexibility, this works.

[abadams]

Did you intend to come back to this commented out code?

[steven-johnson]

Oops. Nope. Scalpel left in patient.

[steven-johnson]

Per comment from @abadams in #3963, should we mark operator halide_buffer_t * with HALIDE_ATTRIBUTE_DEPRECATED here as well?

[abadams]

Probably should port over all the production code we're aware of first, in case it takes a while to get done.

[steven-johnson]

Update: testing this change inside of Google breaks many many things, but mostly because it was formerly legal to do

Buffer<> b;   // no static type!
call_halide(b);

whereas this PR requires a correctly-statically-typed-Buffer at the call site -- which is undoubtedly the correct thing to do in the long run, but it might be simpler to soften this requirement (at least internal to Google) before this can land.

[abadams]

Oh yeah, it would be good to be able to pass dynamically typed buffers. There's a runtime type check done inside the kernel, so it's really just our job here to check the constness of the element type (void vs const void). Checking the type at compile-time is a nice-to-have.

[zvookin]

Ultimately, it would be really handy for us to have a way to generate type casing filters for some uses. Ideally this would avoid a dynamic check if the buffer is statically typed and do the dynamic check and dispatch to a specific implementation if not.

[steven-johnson]

it would be good to be able to pass dynamically typed buffers

That would doable with this; just call .as<whatever>() at the call site.

[steven-johnson]

That would doable with this; just call .as() at the call site.

...except of course for float16, since we don't currently have a runtime type that corresponds to this (Halide::float16_t is compile-time, not runtime), and thus you can't make a statically-correct Buffer<> for this :-(

[abadams]

I think it's desirable to continue to be able to pass dynamically-typed buffers in general without statically declaring their type at the call-site using as()

[steven-johnson]

I disagree, but will go with what the consensus thinks.

[steven-johnson]

Another interesting byproduct of this change (in its current state): currently it's easy to mix Buffer<> and actual halide_buffer_t* in calls; in its current form, this change requires you to use all-of-one or all-of-the-other. Not sure yet if that's a dealbreaker (i.e., if it's ok to require explicit calls to .raw_buffer() in the mixed case). Thoughts?

[abadams]

Mixing seems weird to me, but I would be loathe to disallow it without seeing why someone wrote something that way.

[steven-johnson]

This also turns into a big bag of hurt for a lot of our existing code that was (unintentionally) taking advantage of this bug. This will probably take some hygiene to land.

[steven-johnson]

So after looking at more use of this PR in our code... this can be made to work but is going to be a lot of churn. In particular, we have a surprisingly large amount of code that defines function pointers with halide_buffer_t* arguments and uses them in bottlenecks in a lot of places; the fact that Buffer<> implicitly converts to this makes this work on most places, and adding explicit calls to .raw_buffer() adds a lot of verbosity.

Obvious answer #1 is presumably "Use Buffer<>* instead of halide_buffer_t*", and that's not a bad answer in any event, but here's a thought:

• Define halide_const_buffer_t as a type in HalideRuntime; it's identical to halide_buffer_t except that the host field is const
• keep the implicit Buffer<T> -> halide_buffer_t* and add an implicit Buffer<const T> -> halide_const_buffer_t*
• modify header file generation to require halide_const_buffer_t* for inputs

Pro: would allow for cleaner migration of existing code since inserting .raw_buffer() no longer needed
Con: Doesn't do element-type checking, which is desireable
Con: Actually adds more implicit conversions rather than reducing them