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

Do not check for undefined behavior in abs_diff #842

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

steffenlarsen
Copy link
Contributor

Similar to sycl::abs, sycl::abs_diff now has undefined behavior for unrepresentable results. This patch changes the generated tests for abs_diff to avoid UB, similar to how it was done for abs in #831.

Similar to sycl::abs, sycl::abs_diff now has undefined behavior for
unrepresentable results. This patch changes the generated tests for
abs_diff to avoid UB, similar to how it was done for abs in
KhronosGroup#831.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested a review from a team as a code owner December 7, 2023 16:11
@bader bader added the help wanted Extra attention is needed label Dec 7, 2023
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

This sounds complicated. Would it not possible to do like the unsigned case but casting first to unsigned and adding a bias of - [ ] -min() on each element first?

@steffenlarsen
Copy link
Contributor Author

This sounds complicated. Would it not possible to do like the unsigned case but casting first to unsigned and adding a bias of - [ ] -min() on each element first?

I worry we would run into the same problem causing this problem when computing -min() if it is the minimum value of an integer which is unrepresentable in its signed form.

@keryell
Copy link
Member

keryell commented Dec 14, 2023

I mean for uint8_t for example:

auto au = static_cast<U>(a) + 0x80;
auto bu = static_cast<U>(b) + 0x80;
auto h = (au > bu) ? au : bu;
auto l = (au <= bu) ? au : bu;
return h - l;

So technically 0x80 is static_cast<U>(std::numeric_limits<T>::min())

@steffenlarsen
Copy link
Contributor Author

Is something like https://godbolt.org/z/ejbK9qPdj what you are thinking?

util/math_reference.h Outdated Show resolved Hide resolved
util/math_reference.h Outdated Show resolved Hide resolved
@bader bader requested a review from keryell December 17, 2023 23:05
T h = (a > b) ? a : b;
T l = (a <= b) ? a : b;
return h - l;
// Using two's-complement and that unsigned integer underflow is defined as
// modulo 2^n we get the result by computing the distance based on signed
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, now I understand the code. :-)
But is it signed? It looks like it uses unsigned comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why Steffen says "signed", but would it be enough to just drop this word from the comment to merge it?
P.S. Steffen is on vacation, but we need this PR.

@bader bader requested a review from keryell December 20, 2023 17:27
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Great!
We can always bike-shed the comment later. ;-)

@bader bader merged commit 9691bc0 into KhronosGroup:SYCL-2020 Dec 20, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants