Remove ParamMap (Fix #7121) #7355
Conversation
This PR proposes to remove ParamMap entirely for Halide 16; it was added to provide a threadsafe way to provide parameteres to the JIT, but `compile_to_callable()` now does this in a much less intrusive way. Normally I'd propose deprecating something like this first (eg via ifdef), but in this case it would be extremely intrusive to do so, so this PR just rips off the band-aid and removes it entirely -- feedback welcome on this.
steven-johnson
added
the
release_notes
| @@ -12,11 +12,15 @@ | |||
| using namespace Halide; | |||
| using namespace Halide::Tools; | |||
|
|
|||
| // The Halide compiler is currently not guaranteed to be thread safe. | |||
There was a problem hiding this comment.
It's not? I thought we fixed all the non-threadsafe stuff a long time ago.
There was a problem hiding this comment.
Sadly, no -- we had several tests that were multithreaded (eg simd_op_check) and had to remove all of that because of thread safety issues (e.g. Parameters being used across multiple threads) -- if it's threadsafe now then we don't have any tests that verify this via torture
There was a problem hiding this comment.
Should be fine if no Halide objects are shared across threads. I.e. we don't have any non-threadsafe hidden global state (though temporaries will get non-deterministic names due to atomic counters used to name them).
But yeah the individual objects may have non-threadsafe state, so if you share Halide objects across threads and mutate them bad things will happen.
|
Sure maybe an ifdef is a pain, but can't we mark all the ParamMap-consuming methods with deprecation warnings for a version before ripping them out? |
I guess we could, we'd just have to introduce new overloads for all of them (basically doubling the number of overloads to realize() and infer_input_bounds()), otherwise you'd get deprecation warnings all over the place. IIRC we know of exactly one user for ParamMap in the first place -- we should contact them specifically and get their feedback. |
|
What about adding a deprecation warning to all ParamMap methods except for empty_map() and the default constructor? |
Looks promising, see #7357 |
|
Closing this in favor of #7357 |
This PR proposes to remove ParamMap entirely for Halide 16; it was added to provide a threadsafe way to provide parameteres to the JIT, but
compile_to_callable()now does this in a much less intrusive way.Normally I'd propose deprecating something like this first (eg via ifdef), but in this case it would be extremely intrusive to do so, so this PR just rips off the band-aid and removes it entirely -- feedback welcome on this.