Skip to content

Conversation

@rafbiels
Copy link
Contributor

Description

Ensure the cuFFT and rocFFT backends compile and run with AdaptiveCpp. The changes required are minimal and come from:

  • different names of backend enum values between the compilers (e.g. ext_oneapi_cuda vs cuda)
  • different names of the native command extension
  • no host_task in AdaptiveCpp

Checklist

  • Do all unit tests pass locally? Attach a log.

  • Have you added relevant regression tests? Not needed - existing unit tests cover all the features and work with AdaptiveCpp.

  • Have you included information on how to reproduce the issue (either in a
    GitHub issue or in this PR)? Compile with -DENABLE_MKLCPU_BACKEND=False -DENABLE_MKLGPU_BACKEND=False -DENABLE_CUFFT_BACKEND=True -DBUILD_FUNCTIONAL_TESTS=True -DBUILD_EXAMPLES=True -DONEMATH_SYCL_IMPLEMENTATION=hipsycl -DHIPSYCL_TARGETS=generic -DCMAKE_CXX_COMPILER=acpp

@rafbiels rafbiels requested review from a team as code owners April 24, 2025 14:55
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work!

@anantsrivastava30
Copy link
Contributor

I have two minor things to clarify:

  1. Adaptive release notes mentions the __HIPSYCL*, __hipsycl* etc are in its deprecation phase do you think it would be a good idea to have something like
    #if defined(__ADAPTIVECPP__) || defined(__HIPSYCL__) ... for portability
  2. ONEMATH_SYCL_IMPLEMENTATION=adaptivecpp will fail with “is not known” in the root CMakeLists.txt, is there a separate change that involves this?

Still support older releases with different macro naming
@rafbiels
Copy link
Contributor Author

Thank you for looking into this, @anantsrivastava30!

  1. Adaptive release notes mentions the __HIPSYCL*, __hipsycl* etc are in its deprecation phase do you think it would be a good idea to have something like #if defined(__ADAPTIVECPP__) || defined(__HIPSYCL__) ... for portability

Fair point, I suppose we should still support the old releases (from before the renaming) until we make a decision to drop them (if we ever do). I added the support for old versions in e100870 as suggested, but please note I only tested this with the latest AdaptiveCpp and cannot guarantee the project actually compiles with hipSYCL 0.9.4 or older. I'd like to know if there are any users requiring or strongly interested in old hipSYCL support in oneMath before taking on further work in this direction.

  1. ONEMATH_SYCL_IMPLEMENTATION=adaptivecpp will fail with “is not known” in the root CMakeLists.txt, is there a separate change that involves this?

Not at the moment. The support for AdaptiveCpp/hipSYCL in oneMath is currently somewhat limited and inconsistent. I'm working towards improving the situation in small steps, backend by backend. I do plan addressing your point in the future, though it might need a discussion first on whether we keep supporting both names or only the new one. We had an attempt at moving to the new naming in #543, but sadly the author has left the team before they could finish the work.

Copy link
Contributor

@anantsrivastava30 anantsrivastava30 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick follow-up in e100870, @rafbiels.

  • Re-tested on AdaptiveCpp 24.04 (CUDA backend) – all cufft, rocfftunit tests green.
  • Didn’t try <= hipSYCL 0.9.x, so that path remains best-effort for now.

inline void change_queue_causes_wait(sycl::queue& busy_queue) {
inline void change_queue_causes_wait([[maybe_unused]] sycl::queue& busy_queue) {
// Skip this test in AdaptiveCpp, which doesn't support host_task
#if !defined(__ADAPTIVECPP__) && !defined(__HIPSYCL__)
Copy link
Contributor

@lhuot lhuot Apr 29, 2025

Choose a reason for hiding this comment

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

Why do we need both here and not just !defined(__ADAPTIVECPP__)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was requested in the review from @anantsrivastava30 above. I agreed as the old macros are used throughout the project and we haven't really discussed if we want to drop them. We had an attempt in #543 but the author doesn't work on this any more. I'd be happy to come back to this in a future PR, continuing the work from #543.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, thanks I missed this discussion.

Copy link
Contributor

@lhuot lhuot left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Thanks for the changes!

@Rbiessy Rbiessy merged commit 8964e2d into uxlfoundation:develop Apr 30, 2025
9 checks passed
@Rbiessy Rbiessy mentioned this pull request Jun 5, 2025
@itopinsk itopinsk added this to the v0.8 milestone Jul 1, 2025
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.

6 participants