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

Update oclmath to add AArch64 support. #890

Open
wants to merge 1 commit into
base: SYCL-2020
Choose a base branch
from

Conversation

hvdijk
Copy link
Contributor

@hvdijk hvdijk commented May 1, 2024

This replaces fpcontrol.h and rounding_mode.cpp/.h with the versions from OpenCL CTS. The old version included in SYCL-CTS did not support AArch64, and although it would be possible to just make the smallest possible change to get that to work, it seems like it will be easier for future changes for the OpenCL and SYCL versions of these files to be the same.

@hvdijk hvdijk requested a review from a team as a code owner May 1, 2024 09:53
@hvdijk
Copy link
Contributor Author

hvdijk commented May 1, 2024

CI is showing that the OpenCL CTS versions of these files are not clang-formatted. But if we clang-format them here, we do not avoid the problem that we needlessly have two different versions of these files. My thinking here is that two alternatives are preferable: we could exclude them from clang-formatting, leaving the responsibility with OpenCL CTS to keep it maintainable. Or we could apply the formatting to OpenCL CTS, if its style aligns sufficiently closely with SYCL CTS. Feedback would be welcome here.

This replaces fpcontrol.h and rounding_mode.cpp/.h with the versions
from OpenCL CTS. The old version included in SYCL-CTS did not support
AArch64, and although it would be possible to just make the smallest
possible change to get that to work, it seems like it will be easier for
future changes for the OpenCL and SYCL versions of these files to be the
same.
@hvdijk hvdijk changed the title Update oclmath. Update oclmath to add AArch64 support. May 16, 2024
@hvdijk
Copy link
Contributor Author

hvdijk commented May 16, 2024

Feedback would be welcome here.

Feedback would still be welcome, but I have applied the clang-format changes so that CI should show as green, hopefully making it more likely to get someone to have a look.

@keryell
Copy link
Member

keryell commented Jun 13, 2024

Can anyone remember me why we had these files in the CTS at the first place?

@bader
Copy link
Contributor

bader commented Jul 31, 2024

Can anyone remember me why we had these files in the CTS at the first place?

@keryell, I think it was added to check the accuracy requirements for SYCL math functions in SYCL-1.2.1. @AerialMantis, did I get it right?

@bader
Copy link
Contributor

bader commented Aug 1, 2024

I think we need someone with ARM expertise to review these changes. Considering that we borrowed this code from OpenCL-CTS, I think it's reasonable to ask @kpet to review this change.

@keryell keryell added help wanted Extra attention is needed Agenda To be discussed during a SYCL committee meeting labels Sep 25, 2024
@keryell
Copy link
Member

keryell commented Oct 3, 2024

@kpet nous avons besoin d'aide !

@kpet
Copy link

kpet commented Oct 3, 2024

@keryell I'm on holiday at the moment but I've sent myself a reminder to look into this PR when I get back.

@keryell
Copy link
Member

keryell commented Oct 3, 2024

@keryell I'm on holiday at the moment but I've sent myself a reminder to look into this PR when I get back.

Thanks! There is no emergency since unfortunately this PR is old, but it would be nice to have the feedback from a specialist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda To be discussed during a SYCL committee meeting help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants