Skip to content

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

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

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

Conversation

@jinge90

@jinge90 jinge90 commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

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

jinge90 commented Dec 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@romanovvlad romanovvlad left a comment

Copy link
Copy Markdown
Contributor

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

jinge90 commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

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

jinge90 commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

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

@xtian-github xtian-github left a comment

Copy link
Copy Markdown

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
Comment thread sycl/include/sycl/ext/oneapi/experimental/bfloat16_math.hpp Outdated
Comment thread sycl/include/sycl/ext/oneapi/experimental/bfloat16_math.hpp Outdated
xtian-github
xtian-github approved these changes Dec 15, 2022
View reviewed changes

@xtian-github xtian-github left a comment

Copy link
Copy Markdown

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

xtian-github commented Dec 15, 2022

Copy link
Copy Markdown

@bader Thanks.

@gmlueck

gmlueck commented Dec 20, 2022

Copy link
Copy Markdown
Contributor

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

jinge90 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

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