Skip to content

[SYCL] Add bfloat16 generic implementation for fmax, fmin #7732

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 15, 2022
Merged

[SYCL] Add bfloat16 generic implementation for fmax, fmin #7732

merged 4 commits into from
Dec 15, 2022

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Dec 12, 2022

Signed:sign-off-by: jinge90 [email protected]

@jinge90 jinge90 requested a review from a team as a code owner December 12, 2022 08:19
@jinge90 jinge90 mentioned this pull request Dec 12, 2022
Closed
@jinge90
Copy link
Contributor Author

jinge90 commented Dec 12, 2022 •
edited
Loading

Hi, @JackAKirk and @gmlueck
As we discussed in #7503 , I will move generic implementation for some bfloat16 utils to bfloat16_math.hpp, the first step is fmax and fmin, could you help review?
The corresponding tests are in intel/llvm-test-suite#1444

Thanks very much.

romanovvlad
romanovvlad approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

LGTM

@jinge90
1.Fix return type of isnan(bfloat16)
293fb42 
2. Use sycl::isnan in sycl::ext::oneapi::experimental::complex explicitly
to avoid conflicting with bfloat16 isnan
Signed-off-by: jinge90 <[email protected]>
@jinge90 jinge90 requested a review from romanovvlad December 13, 2022 02:11
@jinge90
Copy link
Contributor Author

jinge90 commented Dec 13, 2022

Hi, @romanovvlad
I update the PR to fix a return type error of isnan and a conflict with sycl complex headers, could you help review again?
Thanks very much.

@jinge90
Copy link
Contributor Author

jinge90 commented Dec 13, 2022

Hi, @JackAKirk and @gmlueck
Kind ping~
Thanks very much.

@jinge90 jinge90 requested a review from xtian-github December 14, 2022 04:54
xtian-github
xtian-github approved these changes Dec 14, 2022
View reviewed changes
Copy link

@xtian-github xtian-github left a comment

Choose a reason for hiding this comment

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

LGTM

bader
bader approved these changes Dec 15, 2022
View reviewed changes
xtian-github
xtian-github approved these changes Dec 15, 2022
View reviewed changes
Copy link

@xtian-github xtian-github left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit eb1ed10 into intel:sycl Dec 15, 2022
@xtian-github
Copy link

@bader Thanks.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 20, 2022

It looks like this PR adds a new public API sycl::ext::oneapi::experimental::isnan(). Was that intentional, or should this be in detail? If it was intentionally added, the extension specification should be updated.

@jinge90
Copy link
Contributor Author

jinge90 commented Dec 21, 2022

It looks like this PR adds a new public API sycl::ext::oneapi::experimental::isnan(). Was that intentional, or should this be in detail? If it was intentionally added, the extension specification should be updated.

Hi, @gmlueck
I think isnan should be added in spec since all other fp format (fp32, fp64, fp16) supports it, I will create a PR to update the spec doc.
Thanks very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@xtian-github xtian-github xtian-github approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

@JackAKirk JackAKirk Awaiting requested review from JackAKirk

@gmlueck gmlueck Awaiting requested review from gmlueck

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jinge90 @xtian-github @gmlueck @bader @romanovvlad