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

Remove old workarounds; Fix build with current hipSYCL #200

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Aug 3, 2023

  • Removes the ComputeCpp CMake integration
  • Removes all #preprocessor workarounds for ComputeCpp
  • Bumps the minimum hipSYCL version to d2bd9fc7 to follow the get_info API change from enum parameters to type parameters (see [SYCL2020] Migrate information descriptors to their respective namespaces AdaptiveCpp/AdaptiveCpp#987) and removes all workarounds for older versions of hipSYCL
  • Removes the subscript_result_t workaround for ComputeCpp's Clang-8 buggy implementation of decltype(auto)
  • Transitions all examples to unnamed kernels (kernel names were only required for CCPP)

This allows building Celerity with the latest hipSYCL (The latest image is not tagged yet due to a CI bug, so 23.04 appears broken even though it passes on a manual trigger).

Unfortunately the DPC++ CI is still broken due to what appears to be UB in the Intel IGC we've recently upgraded to. At some point we should bisect IGC versions to find a working revision and downgrade both our Docker environments and CI runner hosts accordingly.

Partially addresses celerity/meta#20, but some non-trivial workarounds remain that were introduced because of CCPP.

@fknorr fknorr requested a review from psalz August 3, 2023 14:32
@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Check-perf-impact results: (9e900a306dc9f17e4a27439205a7680c)

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

@fknorr fknorr requested a review from PeterTh August 3, 2023 15:32
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, other than one minor remark regarding the examples.
Always nice to get rid of a lot of obsolete code.

examples/convolution/convolution.cc Outdated Show resolved Hide resolved
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.

Feels good!

CMakeLists.txt Outdated Show resolved Hide resolved
@fknorr fknorr force-pushed the remove-old-workarounds branch from 1b0fd66 to 62c7399 Compare August 4, 2023 14:49
@fknorr fknorr self-assigned this Aug 4, 2023
@fknorr fknorr mentioned this pull request Aug 6, 2023
3 tasks
@fknorr fknorr force-pushed the remove-old-workarounds branch from f9e530d to ab8a00c Compare August 7, 2023 12:55
@fknorr fknorr merged commit d1ce304 into master Aug 7, 2023
@fknorr fknorr deleted the remove-old-workarounds branch August 7, 2023 13:25
@fknorr fknorr added this to the 0.4.1 milestone Aug 15, 2023
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