Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

mcourteaux
Copy link
Contributor

@mcourteaux mcourteaux commented May 28, 2024

[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:

Halide/src/IROperator.cpp

Lines 1590 to 1593 in 33d5ba9

Expr unsafe_promise_clamped(const Expr &value, const Expr &min, const Expr &max) {
user_assert(value.defined()) << "unsafe_promise_clamped with undefined value.\n";
Expr n_min_val = min.defined() ? lossless_cast(value.type(), min) : value.type().min();
Expr n_max_val = max.defined() ? lossless_cast(value.type(), max) : value.type().max();

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...

Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the change to IRPrinter should really be done as a separate PR

@mcourteaux
Copy link
Contributor Author

Well, it helps print the error message?

@steven-johnson
Copy link
Contributor

LGTM pending green bots

@steven-johnson steven-johnson self-requested a review June 4, 2024 16:32
Copy link
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failures are unrelated.

@steven-johnson steven-johnson merged commit 46e866d into halide:main Jun 4, 2024
14 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants