halide / Halide

a language for fast, portable data-parallel computation
https://halide-lang.org
Other
5.86k stars 1.07k forks source link

Report useful error to user if the promise_clamp all fails to losslessly cast. #8238

Closed mcourteaux closed 4 months ago

mcourteaux commented 4 months ago

[unsafe_]promise_clamped is sort of broken in a subtle way, which I didn't fix in this PR. I didn't change its functionality, but instead of an internal_assert() triggering, now a useful user error is reported, which allows the user to fix it. This behavior is better than crashing IMO.

The problem is when you for example call: unsafe_promise_clamped(<uint16_t>, 0, <Variable int32_t>). I ran into the situation where I naturally would start from:

counter_output_buf(i) = undef<int32_t>();
counter_output_buf(index_func(rdom)) += 1;

Where counter_output_buf (a GeneratorOutput<Buffer<int32_t>>) counts the amount of times a certain index is observed in index_func. However, index_func is uint16_t in my case. But want to promise that the index I have is within bounds of the buffer, so I did:

counter_output_buf(i) = undef<int32_t>();
counter_output_buf(unsafe_promise_clamped(index_func(rdom), 0, counter_output_buf.dim(0).extent() - 1)) += 1;

This now fails, because the first value is uint16_t, and my upper bound is int32_t, which cannot be losslessly cast to uint16_t, as per: https://github.com/halide/Halide/blob/33d5ba9536f5aa6d3702c2c764b899a8f45b3b62/src/IROperator.cpp#L1590-L1593

The lossless_cast returns an undefined Expr if it fails, which then trigger the internal_assert that makes sure all arguments to a Call are defined.

My point being that all of this feels a little odd behavior of this function. When you want to promise it's clamped, and it actually doesn't do it because of some type mismatch, feels kinda odd. But maybe that's perfectly on purpose and good? Not sure...

mcourteaux commented 4 months ago

Well, it helps print the error message?

steven-johnson commented 4 months ago

LGTM pending green bots