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

Changed pi_context to ur_context_handle_t and include pi.hpp to ur.hpp #546

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

konradkusiak97
Copy link
Contributor

Description

After plugin interface removal patch in intel/llvm, the oneMKL build broke. This PR fixes it by:

  • changing all occurences of #include pi.hpp to ur.hpp
  • changing pi_context to ur_context_handle_t

Also, for the build to work correctly again, this patch is needed:

@hjabird
Copy link
Contributor

hjabird commented Aug 7, 2024

Hi Konrad,
A few questions:

  • Can you provide test logs for everything you've changed
    • cublas, rocblas, cusolver, rocsolver
  • I'm confused about the compiler compatibility right now?

@konradkusiak97
Copy link
Contributor Author

Hi Hugh,

Yes, we've realized this would break compatibility with previous DPC++ releases so I'm working on adding some ifdefs. I'll provide the test logs as well.

@konradkusiak97
Copy link
Contributor Author

konradkusiak97 commented Aug 14, 2024

Sorry for a delay with this as I was sick.

Here are the logs when using 2024.2.1 DPC++:
amd_test_main_lapack_rt.txt
amd_test_main_lapack_ct.txt
amd_test_main_blas_rt.txt
amd_test_main_blas_ct.txt
amd_example_lapack_getrs_usm.txt
amd_example_blas_gemm_usm.txt

nvidia_test_main_lapack_rt.txt
nvidia_test_main_lapack_ct.txt
nvidia_test_main_blas_rt.txt
nvidia_test_main_blas_ct.txt
nvidia_example_lapack_getrs_usm.txt
nvidia_example_blas_gemm_usm.txt

For 2025 DPC++ testing there are some failures with both Nvidia and AMD but I think they are unrelated to this patch. Likely the problem is with the compiler but I'm still investigating those.

@gajanan-choudhary
Copy link
Contributor

Looks like this is the fix for #558 that I just opened.

@Rbiessy Rbiessy requested review from a team August 19, 2024 09:01
@Rbiessy
Copy link
Contributor

Rbiessy commented Aug 20, 2024

@oneapi-src/onemkl-admin this change is needed to get the CI working with a nightly compiler, a second approval would be useful.

Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

This all looks fine to me. Should we have someone from @oneapi-src/onemkl-lapack-write take a quick look at the solvers changes?

@Rbiessy
Copy link
Contributor

Rbiessy commented Aug 20, 2024

Ideally yes but I am planning to merge this tomorrow morning unless I hear an objection.

@Rbiessy Rbiessy merged commit 99325fa into oneapi-src:develop Aug 21, 2024
6 checks passed

// After Plugin Interface removal in DPC++ ur.hpp is the new include
#if __has_include(<sycl/detail/ur.hpp>)
#include <sycl/detail/ur.hpp>

Choose a reason for hiding this comment

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

Please note that instead of removing a footgun you have just reloaded it so it will fire at you once again in upcoming months (or years) when we drop UR in favor of something else.

You should not use implementation details like this in your project. Why aren't use using the official SYCL interop interface, i.e. backend_return_t<your_backend, context>?

I'm also surprised that you are changing the type of that variable without changing any of its uses, meaning that the type doesn't matter for you because it will be erased at some point. Wouldn't it be better to use some opaque type right away to avoid dependency on particular SYCL implementation?

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.

Build with open-source DPC++-LLVM (clang++) broken due to compiler removal of sycl/detail/pi.hpp
6 participants