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

MAINT: remove DAAL CL kernels and GPU interfaces #2816

Merged
merged 48 commits into from
Aug 12, 2024

Conversation

ethanglaser
Copy link
Contributor

@ethanglaser ethanglaser commented Jun 10, 2024

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 intel/scikit-learn-intelex#1770

@ethanglaser
Copy link
Contributor Author

/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
Contributor Author

/intelci: run

@ethanglaser
Copy link
Contributor Author

/intelci: run

Copy link
Contributor

@icfaust icfaust left a comment

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.

.ci/env/apt.sh Outdated Show resolved Hide resolved
cpp/daal/include/services/env_detect.h Show resolved Hide resolved
cpp/daal/src/algorithms/classifier/BUILD Show resolved Hide resolved
Copy link
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
Contributor

Impressive PR, looks good overall.

Additional question on

static const char * zeLoaderName = DAAL_LEVEL_ZERO_LIB_VERSIONED_NAME(libze_loader.so);

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.

Copy link
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.

Copy link
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
Contributor Author

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 oneapi-src:main Aug 12, 2024
14 of 16 checks passed
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