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

Remove assertions in minmax_element binary operator #1593

Merged
merged 2 commits into from
May 17, 2024

Conversation

julianmi
Copy link
Contributor

The current minmax_element implementation requires in-order processing since its binary operator assumes that the index of the first input is lower than that of the second input. This is a performance improvement since we can skip the case when both input values are equal. The in-order processing is guaranteed by following the non-commutative reduce pattern. We currently check the index order with assertions with lead to high overheads when compiled without NDEBUG. This may lead to test timeouts or user code looking to be hanging. I think the assertions are not needed since we enforce them in the in-order reduce pattern and we don't use them in the similar min_element algorithm.

@dmitriy-sobolev
Copy link
Contributor

dmitriy-sobolev commented May 17, 2024

@julianmi, could you remove the assertions in include\oneapi\dpl\pstl\hetero\algorithm_ranges_impl_hetero.h?

Meanwhile, I found no mention if assert can be used in SYCL device code reading its specification.
Given that there is a corresponding extension in oneAPI DPC++ Compiler, I assume that we should avoid using asserts in kernels, or at least use them conditionally.

@julianmi
Copy link
Contributor Author

@julianmi, could you remove the assertions in include\oneapi\dpl\pstl\hetero\algorithm_ranges_impl_hetero.h?

Done.

Meanwhile, I found no mention if assert can be used in SYCL device code reading its specification. Given that there is a corresponding extension in oneAPI DPC++ Compiler, I assume that we should avoid using asserts in kernels, or at least use them conditionally.

I agree with your statement. I looks like assert seems to work for now in kernel code though. Anyways, I think our tests should capture such semantic issue in our kernels (and they do for this specific case).

Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM

@julianmi julianmi merged commit a9aabb2 into main May 17, 2024
20 checks passed
@julianmi julianmi deleted the dev/julianmi/minmax_element_wo_asserts branch May 17, 2024 12:35
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.

3 participants