Skip to content

MAINT: remove DAAL CL kernels and GPU interfaces#2816

Merged
ethanglaser merged 48 commits intouxlfoundation:mainfrom
ethanglaser:dev/eglaser-clkernel-removal
Aug 12, 2024
Merged

MAINT: remove DAAL CL kernels and GPU interfaces#2816
ethanglaser merged 48 commits intouxlfoundation:mainfrom
ethanglaser:dev/eglaser-clkernel-removal

Conversation

@ethanglaser
Copy link
Copy Markdown
Contributor

@ethanglaser ethanglaser commented Jun 10, 2024 •
edited
Loading

Description

Removes all openCL kernels and DAAL GPU functionality. Changes consist of:

  • removal of all daal/*/oneapi/* files and removal/substitution anywhere these are included
  • removal of cpp/daal/src/sycl/*
  • removal of cpp/daal/include/services/internal/sycl/*
  • removal of all DAAL conditionals related to deviceInfo.isCpu (using only true part and deleting context + deviceInfo), gpu_support_checker, and all DAAL execution contexts
  • removal of other daal files with _oneapi_ or _ucapi_ name
  • removal of cpp/daal/include/data_management/data/internal/numeric_table_sycl*.h and related cleanup
  • removal of daal gpu_support_checker and related usage
  • removal of #include "oneapi/dal/backend/interop/common_dpc.hpp" (cleanup from previous context guard removal) and onedal to dal table conversion/interop for data parallel
  • removal of any SYCL/ONEAPI container macros and replacement with non-sycl equivalent
  • removed mention of opencl and sycl from daal BUILD files, replace sycl with engine to maintain necessary dependency
  • removal of DAAL_SYCL_INTERFACE macros
  • remove sklearnex_sycl from python public CI validation

Can be taken out after merge of uxlfoundation/scikit-learn-intelex#1770

@ethanglaser
Copy link
Copy Markdown
Contributor Author

ethanglaser commented Jun 10, 2024

/intelci: run

@ethanglaser ethanglaser changed the title MAINT: remove daal CL kernels MAINT: remove daal CL kernels and GPU interfaces Jun 12, 2024
@ethanglaser ethanglaser changed the title MAINT: remove daal CL kernels and GPU interfaces MAINT: remove DAAL CL kernels and GPU interfaces Jun 12, 2024
@ethanglaser
Copy link
Copy Markdown
Contributor Author

ethanglaser commented Jun 12, 2024

/intelci: run

@ethanglaser
Copy link
Copy Markdown
Contributor Author

ethanglaser commented Jun 13, 2024

/intelci: run

icfaust
icfaust reviewed Jul 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@icfaust icfaust left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I used grep on cpp/daal and showed the following SYCL/sycl mentions in your branch:

cpp/daal/include/services/internal/buffer.h: * @ingroup sycl
cpp/daal/src/services/error_handling.cpp:    // Group of SYCL-related errors -90900..-90999
cpp/daal/include/services/daal_defines.h:#if (defined(__INTEL_COMPILER) || defined(__INTEL_LLVM_COMPILER)) && !defined(SYCL_LANGUAGE_VERSION)
cpp/daal/include/services/daal_defines.h:const int SERIALIZATION_SYCL_SOA_NT_ID            = 3500;
cpp/daal/include/services/daal_defines.h:const int SERIALIZATION_SYCL_CSR_NT_ID            = 3503;
cpp/daal/include/services/daal_defines.h:const int SERIALIZATION_SYCL_HOMOGEN_NT_ID        = 7500;
cpp/daal/include/services/error_indexes.h:    // Group of SYCL-related errors -90900..-90999
cpp/daal/include/services/internal/buffer.h: *  \brief Wrapper for a SYCL* buffer
cpp/daal/include/services/internal/buffer.h: *  or on host/device sides using SYCL* buffer
cpp/daal/include/services/internal/buffer_impl.h: *  <a name="DAAL-CLASS-SERVICES-INTERNAL__SYCLBUFFERIFACE"></a>
cpp/daal/include/services/internal/buffer_impl.h: *  \brief Common interface for SYCL*-backed buffer

Due to the size, my initial review is sort of shallow. Sorry. You can respond with those that should stay and why.

Alexandr-Solovev
Alexandr-Solovev approved these changes Jul 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Alexandr-Solovev Alexandr-Solovev left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Thanks! I added a couple points related replacement sycl containers, probably its not necessary. And also, please take a deep look on CI, probably makes sense to launch nightly on your pr instead of intelci:run

@napetrov
Copy link
Copy Markdown
Contributor

napetrov commented Jul 16, 2024

Impressive PR, looks good overall.

Additional question on
https://github.com/oneapi-src/oneDAL/blob/6ae4f4e8a3883448b275c304e28c5c51f08e12a8/cpp/daal/include/services/internal/sycl/level_zero_module_sycl.h#L59

In theory with removal of openCL we don't need openCL -> SYCL compatibility level anymore and could relay on SYCL compiler handling of L0/openCL instead of us loading libs.
The only potential caveat here might be MKL if FPK lib still have direct dependency on opencl. Might be this should be a separate PR once this would be merged, but we would like to go away from compat layer and direct work with runtimes.

@icfaust icfaust mentioned this pull request Jul 31, 2024
Merged
Vika-F
Vika-F approved these changes Jul 31, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

Let's merge this.

Alexsandruss
Alexsandruss requested changes Aug 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@Alexsandruss Alexsandruss left a comment

Choose a reason for hiding this comment

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

Code conflict solution and rebase are required to get last sklearnex building change.

@ethanglaser
Copy link
Copy Markdown
Contributor Author

ethanglaser commented Aug 2, 2024

The only potential caveat here might be MKL if FPK lib still have direct dependency on opencl. Might be this should be a separate PR once this would be merged, but we would like to go away from compat layer and direct work with runtimes.

Correct, I re-added openCL mentions from original removal since it led to issues with unresolved FPK symbols but I am watching those tickets and will create a (much smaller) follow-up to this one once those can be removed

@ethanglaser ethanglaser merged commit e983f90 into uxlfoundation:main Aug 12, 2024
@icfaust icfaust mentioned this pull request Mar 21, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icfaust icfaust icfaust left review comments

@napetrov napetrov napetrov left review comments

@Vika-F Vika-F Vika-F approved these changes

@Alexsandruss Alexsandruss Alexsandruss approved these changes

@Alexandr-Solovev Alexandr-Solovev Alexandr-Solovev approved these changes

@razdoburdin razdoburdin Awaiting requested review from razdoburdin razdoburdin is a code owner

@ahuber21 ahuber21 Awaiting requested review from ahuber21 ahuber21 is a code owner

@avolkov-intel avolkov-intel Awaiting requested review from avolkov-intel avolkov-intel is a code owner

@samir-nasibli samir-nasibli Awaiting requested review from samir-nasibli

@homksei homksei Awaiting requested review from homksei homksei is a code owner

@maria-Petrova maria-Petrova Awaiting requested review from maria-Petrova

@aepanchi aepanchi Awaiting requested review from aepanchi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ethanglaser @napetrov @icfaust @Vika-F @Alexsandruss @Alexandr-Solovev