Deprecate old-style realize() methods #5676
Conversation
We had 5 extra variants of realize() (for 0-dim thru 4-dim cases); these are a holdover from both having a limit of 4 dimensions (ie, pre-halide_buffer_t) and also from pre-C++11 (ie, passing in initializer-lists of int was less convenient). Let's deprecated these for Halide 12 and remove them in Halide 13. (Yes, this touches a *lot* of existing code.)
steven-johnson
added
the
release_notes
test/correctness/atomics.cpp
Outdated
| @@ -1174,7 +1174,7 @@ void test_async_tuple(const Backend &backend) { | |||
|
|
|||
| // Run 10 times to make sure race condition do not happen | |||
| for (int iter = 0; iter < 10; iter++) { | |||
| Realization out = consumer1.realize(2 * img_size); | |||
| Realization out = consumer1.realize({2} * img_size); | |||
There was a problem hiding this comment.
Yikes. My RegExp-fu for fixing these failed. Let me manually recheck them all.
There was a problem hiding this comment.
Yeah, I missed a lot of stuff. Oops.
|
Only errors are the correctness_interpreter flakes. |
|
Adding a few more folks for review to comment on the concept of this deprecation proposal. |
|
Fortunately, JIT is certainly used less in most of the big codebases we know about, but perhaps this should go to the Gitter community, too, for wider feedback on potential adverse effects from something so fundamental? Are there potentially bad consequences for affected code, or will this always just give a straightforward type error at compile time? It seems more annoying in Python, where the errors won’t appear until runtime. (We could make the APIs diverge slightly with a little sugar at the Python level to still allow the old style interface, and pack the list before passing through to the new C++ interface.) |
Nope, it should always be a compile error for C++.
It's actually a little worse than that: AFAICT, you have to run Python with the right flags (eg I still think it's a minor but useful longterm change. Part of the motivation here (on my part) is to minimize divergence and reduce the API surface area. Having multiple API variants to do the same thing is IMHO an anti-feature; the old C++ API style was present for reasons that no longer apply. And at this point I think that having variadic arguments as part of our public API is something to generally be avoided, except where the language convenience of doing so is absolutely compelling. |
|
I think it's a good change, but I'm not comfortable asserting that because of the python issue, and because I don't use the JIT a whole lot (except for tests). I agree we should check with more people. E.g. maybe @mikewoodworth could weigh in. I think he uses the JIT. |
|
I'm all for simplifying the api. I don't think this will affect us at all, we only ever realize into preallocated buffers. |
In theory, yes. |
|
Any more thoughts on this? (No rush to land it, obviously, but if we have consensus then moving forward seems good) |
|
Any other comments from folks before I land this? I'll wait another day or two. |
|
Failure is unrelated. |
alexreinking
removed
the
release_notes
We had 5 extra variants of realize() (for 0-dim thru 4-dim cases); these are a holdover from both having a limit of 4 dimensions (ie, pre-halide_buffer_t) and also from pre-C++11 (ie, passing in initializer-lists of int was less convenient). Let's deprecate these for Halide 12 and remove them in Halide 13.
(Yes, this touches a lot of existing code.)