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

Add support for SimSYCL (and GCC / C++20 / Sanitizers) #238

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Jan 23, 2024

This adds SimSYCL as a third supported SYCL implementation. SimSYCL executes kernels synchronously on the host and therefore has support for AddressSanitizer and LeakSanitizer, which we can now use to find UB in Celerity from within our CI.

SimSYCL requires C++20, so our code had to be adapted in a few places and 3rd-party dependencies had to be upgraded to be compatible with the new standard.

Until now all our CI builds have been made with Clang derivatives, so I opted for performing the SimSYCL build with GCC to broaden our compiler support at the same time. This required some further patches, either to work around GCC bugs or to be compliant where Clang was overly permissive.

Finally, SimSYCL + AddressSanitizer actually managed to find two use-after-scope errors in the Celerity tests, which were fixed.

Possible follow-up changes

  • Add Windows / macOS CI with SimSYCL
  • Use SimSYCL instead of hipSYCL images for clang-tidy
  • Simulate multiple different systems, heterogeneous GPU configurations, ...
  • Test device selection with SimSYCL instead of the current mocking setup

@fknorr fknorr requested review from psalz and PeterTh January 23, 2024 17:40
@fknorr fknorr self-assigned this Jan 23, 2024
Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

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

Very nice! Brings us much closer to providing a Celerity development experience that works almost "everywhere".

.github/workflows/celerity_ci.yml Show resolved Hide resolved
include/ranges.h Show resolved Hide resolved
include/user_bench.h Show resolved Hide resolved
include/workaround.h Outdated Show resolved Hide resolved
Copy link

Check-perf-impact results: (4c65f1399a47e0eb1340f63004745b17)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/print_utils.h Show resolved Hide resolved
include/print_utils.h Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

test/grid_tests.cc Outdated Show resolved Hide resolved
@fknorr fknorr force-pushed the simsycl branch 3 times, most recently from fd6a234 to 2ee5d4a Compare January 25, 2024 08:17
@fknorr
Copy link
Contributor Author

fknorr commented Jan 25, 2024

Github Actions seems to mark this PR as failing ❌ when the SimSYCL runs emit any warning, so I resolved all of them:

  • GCC was complaining about memcpy'ing over hydration_id which is not a POD type. I've used the opportunity to re-define all numerical _id types as individual POD structs and get rid of the PhantomType template (which I always found annoying because compiler messages would read celerity::detail::PhantomType<size_t, celerity::blah_blah_node_id> instead of just celerity::detail::node_id)
  • I bumped the small_vector dependency to the latest HEAD to fix a GCC -Wmaybe-uninitialized in tests that were copy-constructing small_vectors
  • ... and also some minor valid complaints by GCC

Curiously, the hipSYCL builds still emit -Wunknown-cuda-version but this does not seem to affect the build status.

Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

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

LGTM, I'd maybe squash some of the commits before merging (e.g. all the dependency version bumps).

It's pretty neat to see the times for "Build and Install Celerity" in the CI run for SimSYCL.

test/grid_tests.cc Outdated Show resolved Hide resolved
For GCC / C++20 compatibility
For GCC / C++20 compatibility
For GCC / C++20 compatibility
@fknorr
Copy link
Contributor Author

fknorr commented Jan 25, 2024

LGTM, I'd maybe squash some of the commits before merging (e.g. all the dependency version bumps).

I'd rather keep those separate, they are all relevant to the "internal changelog".

It's pretty neat to see the times for "Build and Install Celerity" in the CI run for SimSYCL.

They do look good, but that's mostly due to ccache. Build times for the other SYCL impls are not much longer. Except hipSYCL + Ubuntu 23.04? I wonder what's going on there.

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

Very cool!

CHANGELOG.md Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
src/backend/CMakeLists.txt Outdated Show resolved Hide resolved
To resolve a GCC -Wmaybe-uninitialized within small_vectors in tests.
This resolves warnings by GCC about memcpy'ing over types with private
members (-Wclass-memaccess).
@fknorr fknorr merged commit de657bb into master Jan 29, 2024
34 checks passed
@psalz psalz deleted the simsycl branch January 29, 2024 11:05
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.

3 participants