Skip to content

[SYCL][UR] Add Windows debug libs to install target #17512

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

Merged
merged 8 commits into from
Apr 22, 2025

Conversation

kbenzie
Copy link
Contributor

@kbenzie kbenzie commented Mar 18, 2025

  1. Add install target for Windows debug libs called install-unified-runtime-libraries which installs only the runtime library subset of targets. This can be used to provide ur_loaderd.dll and ur_adatper_<name>d.dll which link against the multithreaded debug DLL C runtime by using the DEBUG_POSTFIX target property to append d to the library names.
  2. Add UR_USE_DEBUG_POSTFIX option to enable adding the d suffix to library names.
  3. Remove setting redundant OUTPUT_NAMEs as combining DEBUG_POSTFIX=d and then setting LIBRARY_OUTPUT_NAME/RUNTIME_OUTPUT_NAME exposes a bug in CMake where the DEBUG_POSTFIX is ignored for the ur_loader.dll. Removing these redundant property setters fixed the previous issue.
  4. Add debug UR subbuild on Windows ExternalProject compiled in debug mode and with the UR_USE_DEBUG_POSTFIX option enabled.
    • The resulting libraries are then copied to the <build>/bin/<build>/lib directories so they can be used in testing (not yet implemented).
    • Additionally, they are also included in the deploy-sycl-toolchain install target alongside the normally named runtime libraries.
  5. Update ur_proxy_loaderd.dll to use Windows debug libs

@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from 8a462bf to 3c4e12e Compare March 20, 2025 14:45
@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from 613d83b to a448455 Compare March 20, 2025 17:44
@kbenzie kbenzie marked this pull request as ready for review March 21, 2025 10:44
@kbenzie kbenzie requested review from a team as code owners March 21, 2025 10:44
Copy link
Contributor Author

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

@bratpiorka as mentioned in my last email, here's where to hook in the UMF debug postfix option and copy/install the library to the relevant places.

-DUMF_LINK_HWLOC_STATICALLY:BOOL=${UMF_LINK_HWLOC_STATICALLY}
-DUMF_DISABLE_HWLOC:BOOL=${UMF_DISABLE_HWLOC}
# TODO: Enable d suffix in UMF
# -DUMF_USE_DEBUG_POSTFIX=ON
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enable building umfd.dll/umfd.lib in the UR subbuild.

urd_copy_library_to_build(ur_adapter_${adatper}d)
endforeach()
# TODO: Also copy umfd.dll/umfd.lib
# urd_copy_library_to_build(umfd)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy umfd.dll/umfd.lib into the parent <build>/bin/<build>/lib directories for use in testing.

Comment on lines 418 to 420
# TODO: Also install umfd.dll/umfd.lib
# install(
# FILES ${URD_INSTALL_DIR}/bin/umfd.dll
# DESTINATION "bin" COMPONENT unified-memory-framework)
# install(
# FILES ${URD_INSTALL_DIR}/lib/umfd.lib
# DESTINATION "lib${LLVM_LIBDIR_SUFFIX}" COMPONENT unified-memory-framework)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add umfd.dll/umfd.lib to the install target.

Comment on lines 236 to 230
-P ${CMAKE_BINARY_DIR}/cmake_install.cmake
# TODO: Also install debug UMF runtime libraries component
DEPENDS unified-runtime-libraries
# TODO: Add dependency on building debug UMF libraries
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add command to install only umfd.dll/umfd.lib, we don't want a full install here since we only care about the runtime libraries being installed with the debug postfix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll test this today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly working, I've commented on the PR with one issue.

@kbenzie kbenzie changed the title [UR] Add install target for Windows debug libs [SYCL][UR] Add Windows debug libs to install target Mar 21, 2025
@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from a448455 to 468c07c Compare March 21, 2025 11:35
@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 16, 2025

@kbenzie - Is this ready to go in (after a conflict resolution?)

It needs to pull in the UMF changes which I've not tested yet due a few high priority bug reports that have come in this week.

@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from 468c07c to c3e02d1 Compare April 18, 2025 15:08
@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 18, 2025

@kbenzie - Is this ready to go in (after a conflict resolution?)

It needs to pull in the UMF changes which I've not tested yet due a few high priority bug reports that have come in this week.

I tried bumping the UMF_TAG to

commit b3fd6d0b919332455038bb426d6b881bc214c8fb
Merge: e042ee9 9ec0b00
Author: Łukasz Stolarczuk <[email protected]>
Date:   Wed Apr 16 14:55:21 2025 +0200

    Merge pull request #1238 from PatKamin/fix-debug-dll

    Fix building umfd.dll on single-config generators

But got compile errors, @PatKamin / @lukaszstolarczuk is there a UMF pulldown in progress?

@lukaszstolarczuk
Copy link
Contributor

I tried bumping the UMF_TAG to

commit b3fd6d0b919332455038bb426d6b881bc214c8fb
Merge: e042ee9 9ec0b00
Author: Łukasz Stolarczuk <[email protected]>
Date:   Wed Apr 16 14:55:21 2025 +0200

    Merge pull request #1238 from PatKamin/fix-debug-dll

    Fix building umfd.dll on single-config generators

But got compile errors, @PatKamin / @lukaszstolarczuk is there a UMF pulldown in progress?

We've made the final 0.11.0 release (from branch v0.11.x), and I try to pull it into sycl here: #18095

Can you please share what kind of errors you got?

@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 21, 2025

We've made the final 0.11.0 release (from branch v0.11.x), and I try to pull it into sycl here: #18095

I'll test with this commit.

Can you please share what kind of errors you got?

They are on Linux , I didn't try Windows:

FAILED: unified-runtime/source/adapters/level_zero/CMakeFiles/ur_adapter_level_zero.dir/queue.cpp.o
/usr/lib/ccache/c++ -DUR_ENABLE_SANITIZER -DUR_ENABLE_TRACING -DUR_STATIC_LEVEL_ZERO -DUR_VALIDATION_LAYER_SUPPORTED_VERSION=\"0\" -DUR_VERSION=\"0\" -D_DEBUG -D_FORTIFY_SOURCE=2 -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dur_adapter_level_zero_EXPORTS -I/home/benie/Projects/intel/llvm/build-r/unified-runtime/source/adapters/level_zero -I/home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero -I/home/benie/Projects/intel/llvm/build-r/include -I/home/benie/Projects/intel/llvm/llvm/include -I/home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/../.. -I/home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/LevelZeroLoader-Headers -I/home/benie/Projects/intel/llvm/unified-runtime/include -I/home/benie/Projects/intel/llvm/unified-runtime/source/common -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/include -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/ravl -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/critnib -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/provider -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/memspaces -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/memtargets -I/home/benie/Projects/intel/llvm/build-r/_deps/unified-memory-framework-src/src/uthash -I/home/benie/Projects/intel/llvm/build-r/_deps/level-zero-loader-src/include -I/home/benie/Projects/intel/llvm/build-r/_deps/level-zero-loader-src/include/level_zero -I/home/benie/Projects/intel/llvm/build-r/content-exp-headers/level_zero/include -I/home/benie/Projects/intel/llvm/build-r/content-exp-headers/level_zero/include/../.. -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-error -O3 -DNDEBUG -std=c++17 -fPIC -fdiagnostics-color=always -UNDEBUG -Wall -Wpedantic -Wempty-body -Wformat -Wformat-security -Wunused-parameter -fstack-protector-strong -fvisibility=hidden -fcf-protection=full -fstack-clash-protection -fdiagnostics-color=always -MD -MT unified-runtime/source/adapters/level_zero/CMakeFiles/ur_adapter_level_zero.dir/queue.cpp.o -MF unified-runtime/source/adapters/level_zero/CMakeFiles/ur_adapter_level_zero.dir/queue.cpp.o.d -o unified-runtime/source/adapters/level_zero/CMakeFiles/ur_adapter_level_zero.dir/queue.cpp.o -c /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/queue.cpp
In file included from /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/enqueued_pool.hpp:15,
                 from /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/usm.hpp:14,
                 from /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/context.hpp:27,
                 from /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/ur_level_zero.hpp:28,
                 from /home/benie/Projects/intel/llvm/unified-runtime/source/adapters/level_zero/queue.cpp:23:
/home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp: In function ‘umf::pool_unique_handle_t usm::makeDisjointPool(umf::provider_unique_handle_t&&, umf_disjoint_pool_config_t&)’:
/home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp:209:52: error: invalid conversion from ‘const umf_memory_pool_ops_t*’ to ‘umf_memory_pool_ops_t*’ [-fpermissive]
  209 |       umf::poolMakeUniqueFromOps(umfDisjointPoolOps(), std::move(provider),
      |                                  ~~~~~~~~~~~~~~~~~~^~
      |                                                    |
      |                                                    const umf_memory_pool_ops_t*
In file included from /home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp:18:
/home/benie/Projects/intel/llvm/unified-runtime/source/common/umf_helpers.hpp:225:65: note:   initializing argument 1 of ‘auto umf::poolMakeUniqueFromOps(umf_memory_pool_ops_t*, provider_unique_handle_t, void*)’
  225 | static inline auto poolMakeUniqueFromOps(umf_memory_pool_ops_t *ops,
      |                                          ~~~~~~~~~~~~~~~~~~~~~~~^~~
/home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp: In function ‘umf::pool_unique_handle_t usm::makeProxyPool(umf::provider_unique_handle_t&&)’:
/home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp:219:22: error: invalid conversion from ‘const umf_memory_pool_ops_t*’ to ‘umf_memory_pool_ops_t*’ [-fpermissive]
  219 |       umfProxyPoolOps(), std::move(provider), nullptr);
      |       ~~~~~~~~~~~~~~~^~
      |                      |
      |                      const umf_memory_pool_ops_t*
/home/benie/Projects/intel/llvm/unified-runtime/source/common/umf_helpers.hpp:225:65: note:   initializing argument 1 of ‘auto umf::poolMakeUniqueFromOps(umf_memory_pool_ops_t*, provider_unique_handle_t, void*)’
  225 | static inline auto poolMakeUniqueFromOps(umf_memory_pool_ops_t *ops,
      |                                          ~~~~~~~~~~~~~~~~~~~~~~~^~~

@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 21, 2025

We've made the final 0.11.0 release (from branch v0.11.x), and I try to pull it into sycl here: #18095

I'll test with this commit.

No errors with this tag, must be something on UMF main causing them.

@lukaszstolarczuk
Copy link
Contributor

Can you please share what kind of errors you got?

They are on Linux , I didn't try Windows:

/home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp:209:52: error: invalid conversion from ‘const umf_memory_pool_ops_t*’ to ‘umf_memory_pool_ops_t*’ [-fpermissive]
  209 |       umf::poolMakeUniqueFromOps(umfDisjointPoolOps(), std::move(provider),
      |                                  ~~~~~~~~~~~~~~~~~~^~
      |                                                    |
      |                                                    const umf_memory_pool_ops_t*
In file included from /home/benie/Projects/intel/llvm/unified-runtime/source/common/ur_pool_manager.hpp:18:

Och, yeah, these new consts appeared on main as part of changes for "1.0 API readiness" (for future release).
v0.11.0 should be good to use.

@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from c3e02d1 to a1ec5df Compare April 22, 2025 10:51
@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 22, 2025

We've made the final 0.11.0 release (from branch v0.11.x), and I try to pull it into sycl here: #18095

I'll test with this commit.

No errors with this tag, must be something on UMF main causing them.

I've verified that the v0.11.0 tag is working as expected locally. I've not included the bug it in this PR, though so #18095 should also be merged once the internal drop is ready.

@kbenzie
Copy link
Contributor Author

kbenzie commented Apr 22, 2025

As the testing shows, though, these chanages do require the UMF tag update.

kbenzie added 8 commits April 22, 2025 14:52
This patch adds the `install-unified-runtime-libraries` target which
installs only the runtime library subset of targets. This can be used to
provide `ur_loaderd.dll` and `ur_adatper_<name>d.dll` which link against
the multithreaded debug DLL C runtime by using the `DEBUG_POSTFIX`
target property to append `d` to the library names.
Instead of implicitly adding the `d` suffix to library names make this
optional. `DEBUG_POSTFIX` appears to be broken on Windows, the place
it's most useful, as `urinfo.exe` still attempts to load `ur_loader.dll`
when it should be looking for `ur_loaderd.dll`. Switching this over to
`OUTPUT_NAME ${name}d` fixes this.
Looks like combining `DEBUG_POSTFIX=d` and then setting
`LIBRARY_OUTPUT_NAME`/`RUNTIME_OUTPUT_NAME` exposes a bug in CMake where
the `DEBUG_POSTFIX` is ignored for the `ur_loader.dll`. Removing these
redundant propert setters fixed the previous issue.
Using ExternalProject this commit adds a subbuild of UR in compiled in
debug mode and with the `UR_USE_DEBUG_POSTFIX` option enabled. The
resulting libraries are then copied to the `<build>/bin`/`<build>/lib`
directories so they can be used in testing (not yet implemented).
Additionally, they are also included in the `deploy-sycl-toolchain`
install target alongside the normally named runtime libraries.
@kbenzie kbenzie force-pushed the benie/ur-debug-lib-suffix branch from 5ecdb6b to 19f2d9c Compare April 22, 2025 14:22
@kbenzie kbenzie merged commit 9ae8e2f into intel:sycl Apr 22, 2025
33 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.

8 participants