Skip to content

Conversation

@bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Oct 7, 2022

This PR has several fixes that enable building the OpenCL ICD loader and an import library using mingw:

  1. For 32-bit mingw, using a special .def file with stdcall-decorated functions (e.g. function@n). This is needed to generate a proper import library for 32-bit mingw only. For all other builds we will continue to use the same .def file as before.
  2. Change the HKR GUID query from CM_Get_DevNode_PropertyW to CM_Get_DevNode_Registry_PropertyW. This performs the same query but using a function that is in mingw headers. Note, this is the similar flow used by the Vulkan loader (link).
  3. Updates _WIN32_WINNT to 0x0600 (Windows Vista) for InitOnceExecuteOnce.
  4. Minor CMakeFile fix to build an OpenCL.dll instead of a libOpenCL.dll.

Note, I did need to disable OpenCLOn12 via OPENCL_ICD_LOADER_DISABLE_OPENCLON12, so this isn't quite perfect, but it's much better than what we have currently.

Fixes #191.

See also: #11? #130?

See also: KhronosGroup/OpenCL-SDK#58

Do not have a "lib" prefix to be consistent with MSVC.
Silence warnings for stdcall fixup.
@bashbaug bashbaug changed the title WIP: modify HKR enumeration to work with older headers OpenCL ICD Loader Fixes for mingw Oct 15, 2022
@bashbaug bashbaug marked this pull request as ready for review October 15, 2022 05:18
@Kerilk
Copy link
Contributor

Kerilk commented Oct 15, 2022

Awesome work @bashbaug , thanks for cracking this one!

Small questions:

  • did you extract the decorations from a test application? Or compute it from the signature?
  • did you generate the def file?
    • if not should we? (To clarify, I am not saying it should be done in this PR)
    • if yes should we include the generator script? (Again, maybe not in this PR)

@bashbaug
Copy link
Contributor Author

  • did you extract the decorations from a test application? Or compute it from the signature?

I actually didn't need to do either of these, since the mingw linker printed a message when it couldn't find the undecorated name:

Warning: resolving _clEnqueueNDRangeKernel by linking to _clEnqueueNDRangeKernel@36
Warning: resolving _clEnqueueNativeKernel by linking to _clEnqueueNativeKernel@40
Warning: resolving _clEnqueueReadBuffer by linking to _clEnqueueReadBuffer@36
...

I've since cross-checked this against the exports created by MSVC and the output from the "gendef" tool so I'm fairly certain I got them all and they're correct, but it is still a bit of a manual process. If we wanted to be more sure we could auto-generate a "call every API" test file and include it in CI, but that's getting a little ahead of ourselves since we don't even have this config in CI (yet?).

  • did you generate the def file?

No, I looked into this but I didn't find any existing projects I could start from or obvious CMake functionality to help out. I'd still like to come back to ths at some point.

The very best solution IMHO would be to use more __declspec(dllexport)-like functionality to avoid the need for .def files entirely, but I couldn't quite get this to do what I wanted to, either.

We could also look into a solution similar to Mesa, where a single .def file gets processed by a python script, so at least then we don't need to keep two .def files in sync. That's debatably overkill though, since we shouldn't need to modify our .def files very often.

@Kerilk
Copy link
Contributor

Kerilk commented Oct 19, 2022

Thanks for the details @bashbaug . I'll take a shot at generating these files (def + map) using the python/Docs framework once this is merged. It seems the decoration is 4*number of arguments, and if there are exceptions they should be easy to spot with the diff.

@Kerilk
Copy link
Contributor

Kerilk commented Oct 19, 2022

I approved, but I think @jenatali should take a look at the loader/windows/icd_windows_hkr.c modifications just to make sure I didn't overlook something.

Copy link
Contributor

@jenatali jenatali left a comment

Choose a reason for hiding this comment

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

LGTM, though I don't know the CM APIs very well.

@Kerilk
Copy link
Contributor

Kerilk commented Nov 10, 2022

@bashbaug do you want me to press the merge button, or do you think more discussion is needed?

@bashbaug
Copy link
Contributor Author

I think this is ready to go - I've been testing with these changes for some time now and I haven't seen any issues, and the only semi-risky part is the specific .def file, which is mingw specific.

@Kerilk Kerilk merged commit 96c536e into KhronosGroup:main Nov 11, 2022
MehdiChinoune added a commit to MehdiChinoune/MINGW-packages that referenced this pull request Feb 7, 2023
MehdiChinoune added a commit to MehdiChinoune/MINGW-packages that referenced this pull request Feb 7, 2023
MehdiChinoune added a commit to MehdiChinoune/MINGW-packages that referenced this pull request Feb 7, 2023
MehdiChinoune added a commit to MehdiChinoune/MINGW-packages that referenced this pull request Feb 7, 2023
MehdiChinoune added a commit to msys2/MINGW-packages that referenced this pull request Feb 8, 2023
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.

difficulties building with 32-bit mingw-i686

3 participants