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

Rename hipSYCL to AdaptiveCpp, update CI and regenerate filter #874

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

psalz
Copy link
Contributor

@psalz psalz commented Feb 21, 2024

NOTE: This includes @fknorr's recent PRs (#870, #871, #872, #873), so those should be merged first.

This renames (hopefully) all uses of hipSYCL to AdaptiveCpp and updates the version checked against in CI to the latest develop commit (the previous version was over 2 years old at this point).

I've also gone ahead and re-generated the CI exclude filters: AdaptiveCpp is now able to compile 7 additional categories (device, exception_handling, image, marray, nd_range, range and vector_alias), while failing 2 new categories (group_functions and optional_kernel_features, although I believe those didn't even exist yet the last the filter was generated).

I had to bump our CI docker container base image to Ubuntu 22.04, as the GCC included in 20.04 is too old for current AdaptiveCpp.

I've also taken the liberty of removing the long deprecated Intel_SYCL option for SYCL_IMPLEMENTATION.

cc @illuhad

Closes #796.

@psalz psalz requested review from bader and a team as code owners February 21, 2024 16:28
@bader
Copy link
Contributor

bader commented Feb 21, 2024

NOTE: This includes @fknorr's recent PRs (#870, #871, #872, #873), so those should be merged first.

I'm not sure it's a good idea. These PRs have some unresolved comments and pre-commit failures, which will take time to resolve. Requesting them to be merge first blocks other useful work, which is not directly related to adding SimSYCL support.

I think we can merge fix for #796 and remove the Intel_SYCL separately.

@psalz
Copy link
Contributor Author

psalz commented Feb 22, 2024

I'm not sure it's a good idea. These PRs have some unresolved comments and pre-commit failures, which will take time to resolve. Requesting them to be merge first blocks other useful work, which is not directly related to adding SimSYCL support.

I think we can merge fix for #796 and remove the Intel_SYCL separately.

Are we in a hurry to get this merged? IMO waiting another week or two to get those issues resolved doesn't really make a difference at this point. My main goal for today's call was to assess AdaptiveCpp's current CTS coverage, and not including @fknorr's fixes will negatively affect that.

@keryell
Copy link
Member

keryell commented Sep 10, 2024

How do we move on this?

@keryell
Copy link
Member

keryell commented Sep 25, 2024

@fknorr Some merge conflicts.
We could use also Ubuntu 24.04 to have some recent compilers out of the box?

@psalz psalz force-pushed the hipsycl-to-adaptivecpp branch 2 times, most recently from 58f7eb3 to 83188f5 Compare December 14, 2024 12:46
@bader
Copy link
Contributor

bader commented Dec 14, 2024

I've also taken the liberty of removing the long deprecated Intel_SYCL option for SYCL_IMPLEMENTATION.

This part has been removed from the PR.

docker/common/Dockerfile Outdated Show resolved Hide resolved
Newer versions of AdaptiveCpp (hipSYCL) require GCC >= 10.
For these spellings
  hipSYCL -> AdaptiveCpp
  HIPSYCL -> ADAPTIVECPP
  hipsycl -> adaptivecpp
@psalz psalz force-pushed the hipsycl-to-adaptivecpp branch from 83188f5 to af2da6f Compare December 16, 2024 11:20
@psalz
Copy link
Contributor Author

psalz commented Dec 16, 2024

Well this has taken way too long, sorry about that. I've re-done all of the text replacements and updated the AdaptiveCpp image to a recent commit. It looks like there's fewer test categories compiling now than when I originally opened the PR, but I haven't checked any of the errors, so there might be some low hanging fruit.

@bader bader requested review from bader and illuhad December 16, 2024 16:27
-DWITH_ACCELERATED_CPU=OFF \
-DWITH_CUDA_BACKEND=OFF \
-DWITH_ROCM_BACKEND=OFF \
-DWITH_OPENCL_BACKEND=OFF \
Copy link
Contributor

Choose a reason for hiding this comment

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

For #1012, we should decide for whether we want an LLVM-based build. In this context, I wonder whether there is any issue or challenge enabling the compiler features?

Since AdaptiveCpp even when only targeting CPU now typically relies on LLVM (via --acpp-targets=generic by either JIT-compiling for OpenMP or OpenCL), we could use such a build without having to set up CUDA or ROCm and without needing GPU hardware.

As most users also are going to be using --acpp-targets=generic, that might be useful to have in CTS CI, if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@illuhad, I managed to install LLVM and successfully built default version of AdaptiveCpp. Unfortunately, I can't merge #1012 because SYCL-CTS code base relies on hipSYCL. I think this PR resolve the issue blocking #1012.
Could you please approve this PR and review #1012?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest to merge this PR first and then look into switching to SSCP in a follow up. It should only require changes to the Dockerfile (hopefully), and by merging this now we can get all the string replacements out of the way before they accrue too many conflicts with other PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. @illuhad?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, okay for me!

@bader bader merged commit 011a98a into main Dec 20, 2024
9 checks passed
@bader bader deleted the hipsycl-to-adaptivecpp branch December 20, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump hipSYCL version and regenerate filters
4 participants