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

Fix default device wrap native function #6310

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

OmarEmaraDev
Copy link
Contributor

Currently, an attempt to call device_wrap_native on a target that uses
the default device wrap native function will result in an error of type
halide_error_device_interface_no_device, namely in the OpenGLCompute and
the Hexagon targets. This happens because the default wrap native
function calls debug_log_and_validate_buf after the device_interface is
set in halide_device_wrap_native but before the device handle is set,
which is validated as a bad state.

This patch removes the validation call and adds an assert for the handle
much like the other wrap_native implementations in other targets.

Currently, an attempt to call device_wrap_native on a target that uses
the default device wrap native function will result in an error of type
halide_error_device_interface_no_device, namely in the OpenGLCompute and
the Hexagon targets. This happens because the default wrap native
function calls debug_log_and_validate_buf after the device_interface is
set in halide_device_wrap_native but before the device handle is set,
which is validated as a bad state.

This patch removes the validation call and adds an assert for the handle
much like the other wrap_native implementations in other targets.
Copy link
Member

@abadams abadams left a comment

Choose a reason for hiding this comment

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

Looks correct to me. Thanks for the fix!

@OmarEmaraDev
Copy link
Contributor Author

Perhaps this was missed, do I need to do anything else for this pull request?

@abadams abadams merged commit 5e374bc into halide:master Oct 25, 2021
@abadams
Copy link
Member

abadams commented Oct 25, 2021

Yes it was missed. Thanks for the ping.

@steven-johnson
Copy link
Contributor

Update: this is causing some issues downstream, and I wonder at the logic of this change: halide_assert() isn't a debug-only assertion (it will just abort() if the condition fails, in all build modes)... so the error checking on the next line will never happen. This seems bad and wrong.

@steven-johnson
Copy link
Contributor

(side note: we should rename halide_assert() to something else, since IMHO it implies "debug mode only or at least not in full opt builds", which is not the case)

steven-johnson added a commit that referenced this pull request Nov 3, 2021
This was inserted in #6310, probably mistakenly, since `halide_assert()` in the Halide runtime is *not* a debug-only assertion). Instead of a controlled runtime failure, we just abort, which is not OK.
steven-johnson added a commit that referenced this pull request Nov 3, 2021
This was inserted in #6310, probably mistakenly, since `halide_assert()` in the Halide runtime is *not* a debug-only assertion). Instead of a controlled runtime failure, we just abort, which is not OK.
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.

4 participants