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

Fixed parser of OpenCL 3.0 for NVIDIA GPU #2001

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

redradist
Copy link

No description provided.

@redradist redradist changed the title Fixed parser of OpenCL for NVIDIA GPU Fixed parser of OpenCL 3.0 for NVIDIA GPU Jul 20, 2024
@vpirogov vpirogov added this to the v3.6 milestone Jul 22, 2024
Copy link
Contributor

@densamoilov densamoilov left a comment

Choose a reason for hiding this comment

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

Can you please elaborate on the reasoning behind this PR?

@vpirogov
Copy link
Member

@redradist,

Adding 3.0 to the list of OpenCL version makes sense. We are still looking for your comments on the rationale for remaining changes.

@vpirogov vpirogov added the platform:gpu-nvidia Codeowner: @oneapi-src/onednn-gpu-nvidia label Jul 26, 2024
@redradist
Copy link
Author

redradist commented Jul 28, 2024

@vpirogov @densamoilov Because this code successfully detect version of OpenCL library, but this one with CMAKE_CHECK_SYMBOLS cannot, even with defined variables to include dirs and link libraries:

...

function(_FIND_OPENCL_VERSION)
  include(CheckSymbolExists)
  include(CMakePushCheckState)
  set(CMAKE_REQUIRED_QUIET ${OpenCL_FIND_QUIETLY})
  set(CMAKE_REQUIRED_INCLUDES ${OpenCL_INCLUDE_DIR})
  set(CMAKE_REQUIRED_LIBRARIES ${OpenCL_LIBRARY})

  CMAKE_PUSH_CHECK_STATE()
  foreach(VERSION "3_0" "2_2" "2_1" "2_0" "1_2" "1_1" "1_0")
    if(APPLE)
      CHECK_SYMBOL_EXISTS(
        CL_VERSION_${VERSION}
        "${OpenCL_INCLUDE_DIR}/Headers/cl.h"
        OPENCL_VERSION_${VERSION})
    else()
      CHECK_SYMBOL_EXISTS(
        CL_VERSION_${VERSION}
        "${OpenCL_INCLUDE_DIR}/CL/cl.h"
        OPENCL_VERSION_${VERSION})
    endif()

    message(STATUS "OPENCL_VERSION_${VERSION} = ${OPENCL_VERSION_${VERSION}}")

    if(OPENCL_VERSION_${VERSION})
      string(REPLACE "_" "." VERSION "${VERSION}")
      set(OpenCL_VERSION_STRING ${VERSION} PARENT_SCOPE)
      string(REGEX MATCHALL "[0-9]+" version_components "${VERSION}")
      list(GET version_components 0 major_version)
      list(GET version_components 1 minor_version)
      set(OpenCL_VERSION_MAJOR ${major_version} PARENT_SCOPE)
      set(OpenCL_VERSION_MINOR ${minor_version} PARENT_SCOPE)
      break()
    endif()
  endforeach()
  CMAKE_POP_CHECK_STATE()
endfunction()

...

For CL_VERSION_${VERSION} inside file /usr/include/CL/cl.h (not even in /usr/include/CL/cl_version.h), defines are empty ...
OPENCL_VERSION_${VERSION} is always empty, due to possible missed defines. There are ifdef instead of define with number.
Maybe due to this it cannot detection library version

CL_VERSION_${VERSION}
"${OpenCL_INCLUDE_DIR}/Headers/cl.h"
OPENCL_VERSION_${VERSION})
foreach(VERSION "3_0" "2_2" "2_1" "2_0" "1_2" "1_1" "1_0")
Copy link
Contributor

@densamoilov densamoilov Jul 31, 2024

Choose a reason for hiding this comment

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

(random place)

There are a few things that look suspicious to me:

  • We didn't implement FindOpenCL.cmake module, we've taken it from CMake (see here) and it still uses CHECK_SYMBOL_EXISTS and the community seems to be fine with it
  • The version macros are standardized by the OpenCL specification so if the macros are missing then it likely means that there is something wrong with the OpenCL implementation you use
  • It should be fine to check cl.h header as it includes cl_version.h header and CHECK_SYMBOL_EXISTS does perform a preprocessing

Given all this, I think the issue you encounter is somewhere on your end.

Copy link
Author

@redradist redradist Aug 7, 2024

Choose a reason for hiding this comment

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

@densamoilov Here is my OpenCL files for NVIDIA GPU:
cl_files.zip

It looks fine for me

@vpirogov vpirogov modified the milestone: v3.6 Sep 19, 2024
@vpirogov
Copy link
Member

Considering the discussion the best way forward is updating FindOpenCL.cmake from upstream.

@vpirogov vpirogov requested a review from a team as a code owner November 1, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:gpu-nvidia Codeowner: @oneapi-src/onednn-gpu-nvidia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants