Skip to content

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

Merged
bader merged 4 commits intointel:syclfrom
jinge90:add_bfloat16_fmax_fmin
Dec 15, 2022
Merged

[SYCL] Add bfloat16 generic implementation for fmax, fmin#7732
bader merged 4 commits intointel:syclfrom
jinge90:add_bfloat16_fmax_fmin

Conversation

@jinge90
Copy link
Contributor

@jinge90 jinge90 commented Dec 12, 2022

Signed:sign-off-by: jinge90 ge.jin@intel.com

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

@jinge90
[SYCL] Add bfloat16 generic implementation for fmax, fmin
f7d6d06 
Signed:sign-off-by: jinge90 <ge.jin@intel.com>
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 <ge.jin@intel.com>
@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

xtian-github commented Dec 15, 2022

@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

@JackAKirk JackAKirk Awaiting requested review from JackAKirk

@gmlueck gmlueck Awaiting requested review from gmlueck

+1 more reviewer

@romanovvlad romanovvlad romanovvlad approved these changes

Reviewers whose approvals may not affect merge requirements

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