Skip to content

[SYCL][libclc][CUDA] Add --ffast-math support #5801

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
Apr 25, 2022
Merged

[SYCL][libclc][CUDA] Add --ffast-math support #5801

merged 4 commits into from
Apr 25, 2022

Conversation

pgorlani
Copy link
Contributor

@pgorlani pgorlani commented Mar 14, 2022 •
edited
Loading

This patch allows the --ffast-math compiler flag to substitute the regular genfloatf math built-ins with their ::native versions.

Moreover, this patch completes the support of natives built-ins for libclc/ptx-nvidiacl connecting them with the __nv_fast functions present in libdevice. If a fast function is not available in libdevice the corresponding nvvm intrinsic is used.

Tests in intel/llvm-test-suite#919

@pgorlani
[SYCL][libclc][CUDA] Add --ffast-math support
d6f4a89 
This patch allows the `--ffast-math` compiler
flag to substitute the regular `genfloatf` math
built-ins with their `::native` versions.

Moreover, this patch completes the support of natives
built-ins for `libclc/ptx-nvidiacl` connecting
them with the `__nv_fast` functions present in
libdevice. If a fast function is not available in
libdevice the corresponding `nvvm` intrinsic is used.
@pgorlani pgorlani requested review from a team and bader as code owners March 14, 2022 09:45
@pgorlani pgorlani requested a review from v-klochkov March 14, 2022 09:45
@pgorlani pgorlani mentioned this pull request Mar 14, 2022
Merged
Fznamznon
Fznamznon previously approved these changes Mar 14, 2022
View reviewed changes
bader
bader reviewed Mar 14, 2022
View reviewed changes
mlychkov
mlychkov previously approved these changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@mlychkov mlychkov left a comment

Choose a reason for hiding this comment

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

llvm changes LGTM.

smanna12
smanna12 previously approved these changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

FE changes LGTM.

@s-kanaev
Copy link
Contributor

/verify with intel/llvm-test-suite#919

@pgorlani pgorlani requested a review from bader March 29, 2022 08:59
bader
bader previously approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Recovering my approve.
@pgorlani, to get this PR merged, you should ping @intel/llvm-reviewers-runtime team and investigate why new test added by intel/llvm-test-suite#919 doesn't pass.

@pgorlani
Copy link
Contributor Author

Hi @bader, thanks for the prompt answer!

and investigate why new test added by intel/llvm-test-suite#919 doesn't pass.

Are you referring to the outcome of /verify with intel/llvm#5801? How can I see those results?

If not, the test introduced by intel/llvm-test-suite#919 is expected to fail if it is run on a branch that doesn't support the --ffast-math flag, i.e., it passes just in case #5801 is applied.

@bader
Copy link
Contributor

bader commented Mar 29, 2022

and investigate why new test added by intel/llvm-test-suite#919 doesn't pass.

Are you referring to the outcome of /verify with intel/llvm#5801? How can I see those results?

If not, the test introduced by intel/llvm-test-suite#919 is expected to fail if it is run on a branch that doesn't support the --ffast-math flag, i.e., it passes just in case #5801 is applied.

I see that there were two runs:

  1. From this PR - [SYCL][libclc][CUDA] Add --ffast-math support #5801 (comment) - logs can be found here or click on "Details" for Jenkins/verify-test-suite-logs.
  2. From [SYCL] Add --ffast-math tests llvm-test-suite#919 - [SYCL] Add --ffast-math tests llvm-test-suite#919 (comment) - logs can be found here or click on "Details" for Jenkins/verify-test-suite-logs.

@pgorlani pgorlani dismissed stale reviews from bader, smanna12, mlychkov, and Fznamznon via ffad8ef March 30, 2022 13:14
@pgorlani pgorlani requested a review from a team as a code owner March 30, 2022 13:14
bader
bader previously approved these changes Mar 30, 2022
View reviewed changes
smanna12
smanna12 previously approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

FE changes LGTM

@pgorlani
Copy link
Contributor Author

Thank you so much for the info, @bader.

I had missed the failings in Windows. I pushed a commit that should fix them.

mlychkov
mlychkov previously approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@mlychkov mlychkov left a comment

Choose a reason for hiding this comment

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

LLVM changes LGTM.

mlychkov
mlychkov reviewed Mar 30, 2022
View reviewed changes
@pgorlani @mlychkov
Update llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
d2d9055 
Remove empty line.

Co-authored-by: Mikhail Lychkov <[email protected]>
@pgorlani pgorlani dismissed stale reviews from mlychkov, smanna12, and bader via d2d9055 March 30, 2022 13:29
@pgorlani @mlychkov
Update llvm/include/llvm/IR/IntrinsicsNVVM.td
74ad3d1 
Remove empty line.

Co-authored-by: Mikhail Lychkov <[email protected]>
smanna12
smanna12 approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

FE changes LGTM

@pgorlani
Copy link
Contributor Author

/verify with intel/llvm-test-suite#919

@pgorlani
Copy link
Contributor Author

All the tests passed.
Jenkins/Pre-ci-logs failed for Assert/assert_in_simultaneously_multiple_tus.cpp, which is now disabled.

Ping @intel/llvm-reviewers-runtime

@pgorlani
Copy link
Contributor Author

Ping @v-klochkov

@pgorlani
Copy link
Contributor Author

Thanks to everybody!

Would it be possible to merge this PR?

@againull againull self-requested a review April 25, 2022 15:01
@againull againull merged commit 0f0c5d1 into intel:sycl Apr 25, 2022
@jingwan2 jingwan2 mentioned this pull request Apr 27, 2022
Closed
steffenlarsen pushed a commit to intel/llvm-test-suite that referenced this pull request Jun 24, 2022
@pgorlani
[SYCL] Add --ffast-math tests (#919)
773b61e 
This patch adds tests for intel/llvm#5801
@JackAKirk JackAKirk mentioned this pull request Sep 15, 2022
Merged
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@pgorlani
[SYCL] Add --ffast-math tests (intel/llvm-test-suite#919)
7506be1 
This patch adds tests for intel#5801
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

@Fznamznon Fznamznon Fznamznon approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@againull againull againull approved these changes

+2 more reviewers

@mlychkov mlychkov mlychkov approved these changes

@smanna12 smanna12 smanna12 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.
9 participants
@pgorlani @s-kanaev @bader @Fznamznon @v-klochkov @mdtoguchi @againull @mlychkov @smanna12