[SYCL] Support build with shared libzstd

[dvrogozh]

Distros of Fedora family package only shared libzstd build against which was not fully supported. This commit addresses the gap in a similar way as it's already handled in few other cases in llvm. See for reference:

llvm/llvm/lib/Support/CMakeLists.txt

Lines 28 to 38 in b09acde

	if(LLVM_ENABLE_ZSTD)
	if(TARGET zstd::libzstd_shared AND NOT LLVM_USE_STATIC_ZSTD)
	set(zstd_target zstd::libzstd_shared)
	else()
	set(zstd_target zstd::libzstd_static)
	endif()
	endif()
	if(LLVM_ENABLE_ZSTD)
	list(APPEND imported_libs ${zstd_target})
	endif()

and:

llvm/lld/ELF/CMakeLists.txt

Lines 9 to 18 in b09acde

	if(LLVM_ENABLE_ZSTD)
	if(TARGET zstd::libzstd_shared AND NOT LLVM_USE_STATIC_ZSTD)
	set(zstd_target zstd::libzstd_shared)
	else()
	set(zstd_target zstd::libzstd_static)
	endif()
	endif()
	if(LLVM_ENABLE_ZSTD)
	list(APPEND imported_libs ${zstd_target})

Fixes: #22310

CC: @sarnex

[sarnex]

@uditagarwal97 Do you mind handling the review of this one? I remember you worked on the zstd support and there was some weird behavior requiring us to use the static one for reasons I don't totally remember. Thanks!

[uditagarwal97]

@dvrogozh The reason why we only supported building DPCPP with static zstd initially was to ease distribution of DPCPP by not requiring customers to install zstd separately (if we would have used shared zstd library). While installing zstd on Linux is trivial, that's not the case on Windows. So, in this PR, please put shared zstd build behind a flag that user can opt-in so that the default DPCPP build remains the same.

[dvrogozh]

So, in this PR, please put shared zstd build behind a flag that user can opt-in so that the default DPCPP build remains the same.

Such flag already exists and is inherited from original vllm - that's LLVM_USE_STATIC_ZSTD. This PR does not change current behavior, but just fixes the issue for the build path when user opts-in for the shared zstd. By default the build script for the compiler forces static zstd already:

llvm/buildbot/configure.py

Line 185 in 7561294

"-DLLVM_USE_STATIC_ZSTD=ON",

[uditagarwal97]

So, in this PR, please put shared zstd build behind a flag that user can opt-in so that the default DPCPP build remains the same.

Such flag already exists and is inherited from original vllm - that's LLVM_USE_STATIC_ZSTD. This PR does not change current behavior, but just fixes the issue for the build path when user opts-in for the shared zstd. By default the build script for the compiler forces static zstd already:

llvm/buildbot/configure.py

Line 185 in 7561294

"-DLLVM_USE_STATIC_ZSTD=ON",

I don't think we can rely on LLVM_USE_STATIC_ZSTD because it is not enabled by default and not of all DPCPP users use the buildbot/configure.py script. Can we have another CMake flag or maybe do something like the following in sycl/CMakeFile:

#ifndef LLVM_USE_STATIC_ZSTD
#define LLVM_USE_STATIC_ZSTD 1
#endif

#if LLVM_USE_STATIC_ZSTD 
// Use static zstd
#else
// Use shared
#endif

And for Fedora build you can explicitly pass -DLLVM_USE_STATIC_ZSTD=0.

[dvrogozh]

I don't think we can rely on LLVM_USE_STATIC_ZSTD because it is not enabled by default and not of all DPCPP users use the buildbot/configure.py script. Can we have another CMake flag <...>

@uditagarwal97, this seem to reinvent the wheel. LLVM_USE_STATIC_ZSTD is a standard flag available and well established in LLVM. Why it is suddenly not enough for the derivative DPC++ compiler? It gives a full control to the user already. As I pointed out, those users who are using buildbot/configure.py already are on a static zstd path as it's hardcoded in the script. Those who are using cmake directly already have all the control they need thru the flags exposed by LLVM. I would also argue about the "default" behavior. What is that exactly? Note that the only documented way to build DPC++ compiler is thru buildbot/configure.py which makes everything else including direct usage of cmake non-default.

[uditagarwal97]

I would also argue about the "default" behavior. What is that exactly?

Even if the user doesn't set LLVM_USE_STATIC_ZSTD, the current DPCPP build always links zstd static library. But, after your PR, that behavior will change and now it will start linking against shared zstd library, if it is available. The consequence of it will be severe - we will end up unintentionally introducing a runtime dependency to DPCPP.

So, what are our options?
(1) Ask all DPCPP customers who build DPCPP from source to change their existing build scripts and add -DLLVM_USE_STATIC_ZSTD flag while configuring DPCPP
(2) Set LLVM_USE_STATIC_ZSTD=1 by default while allowing user to override it if they intentionally want to have zstd as a runtime dependency of DPCPP.

Which of these options do you think will be most comfortable for our customers?

Regarding buildbot/configure.py script, here's what our documentation (https://github.com/intel/llvm/blob/sycl/sycl/doc/GetStartedGuide.md#build-dpc-toolchain) says:

Note that the CMake variables set by default by the configure.py script are the ones commonly used by DPC++ developers and might not necessarily suffice for your project-specific needs.

Not all of our users use configure.py script and we mostly use this script for daily development and CI.

[dvrogozh]

@uditagarwal97, with all respect, but what you suggest and highlight does not make sense to me. Per my opinion the proposal you are making is an attempt to workaround initially wrong implementation done in the SYCL library CMake file motivated by highly controversial idea of convenience to use static library and in direct contradiction with the established practice in the upstream LLVM. I strongly recommend to reconsider your recommendation and align behavior with the upstream LLVM practice. Note that right now is a very good time to do so considering that v7.0.0 is a next major release series of DPC++ compiler which is not yet released, so changes in the default behavior if any are totally expected at this very moment. I still don't see however which changes in "default" behavior you are talking about considering that LLVM+cmake already have everything needed to tune the build, i.e. BUILD_SHARED_LIBS, LLVM_ENABLE_ZSTD and LLVM_USE_STATIC_ZSTD.

I would also like to point out that even following your idea we will need to patch not only SYCL cmake, but also LLVM and LLD cmakes as well. Please, review the respective code pieces I've pointed to in the PR description.

I am absolutely not ok to follow your recommendation to change the PR I have proposed. That is a wrong thing to do per my opinion. This would complicate the code and diverge from the established practice to handle zstd library in upstream LLVM. If you chose not to accept the contribution proposed in this PR, I do expect that you provide the resolution for the following issue which is one of the issues blocking CI enabling on pytorch side for intel/llvm compiler path:

• sycl: zstd_STATIC_LIBRARY is NOTFOUND building on almalinux 8 #22310

[uditagarwal97]

@uditagarwal97, with all respect, but what you suggest and highlight does not make sense to me. Per my opinion the proposal you are making is an attempt to workaround initially wrong implementation done in the SYCL library CMake file motivated by highly controversial idea of convenience to use static library and in direct contradiction with the established practice in the upstream LLVM. I strongly recommend to reconsider your recommendation and align behavior with the upstream LLVM practice.

I'm not sure what was not clear in my previous comment and I would disagree about the initial implementation being incorrect. Upstream LLVM has an optional runtime dependency on zstd, we don't want that in DPCPP. We don't want our users to have zstd installed on their system just to use Intel compiler. And for what it is worth, I'd like to highlight that install zstd on Windows and older RHEL OS is not trivial. User would have to build/download package and add to environment manually. The divergence from upstream LLVM is done solely for the convenience of our users 😄

Note that right now is a very good time to do so considering that v7.0.0 is a next major release series of DPC++ compiler which is not yet released, so changes in the default behavior if any are totally expected at this very moment. I still don't see however which changes in "default" behavior you are talking about considering that LLVM+cmake already have everything needed to tune the build, i.e. BUILD_SHARED_LIBS, LLVM_ENABLE_ZSTD and LLVM_USE_STATIC_ZSTD.

Quote from my previous comment about current DPCPP build behavior:

Even if the user doesn't set LLVM_USE_STATIC_ZSTD, the current DPCPP build always links zstd static library. But, after your PR, that behavior will change and now it will start linking against shared zstd library, if it is available.

All I want is for this behavior to stay the same. You can achieve that by setting LLVM_USE_STATIC_ZSTD in sycl/CMakeList if user hasn't already set this flag. This way, for Fedora and other OS on which you want DPCPP to build with shared zstd, you can just pass LLVM_USE_STATIC_ZSTD=0 flag. All other users will stay unaffected.

I would also like to point out that even following your idea we will need to patch not only SYCL cmake, but also LLVM and LLD cmakes as well. Please, review the respective code pieces I've pointed to in the PR description.

Sorry, how? Why would we need to change LLVM/LLD Cmake files? On Fedora, if we build DPCPP with LLVM_USE_STATIC_ZSTD=0 flag, LLVM, LLD, SYCL, will all use dynamic zstd.

I am absolutely not ok to follow your recommendation to change the PR I have proposed. That is a wrong thing to do per my opinion. This would complicate the code and diverge from the established practice to handle zstd library in upstream LLVM.

I am not the sole maintainer/reviewer in intel/llvm, so if anyone else feel comfortable with the change in this PR, they can approve it. I'd have no objections to it. Personally, based on my prior experience enabling zstd in intel/llvm, I think this change will bring unnecessary inconvenience to our users, and so I'm not comfortable approving it myself.

If you chose not to accept the contribution proposed in this PR, I do expect that you provide the resolution for the following issue which is one of the issues blocking CI enabling on pytorch side for intel/llvm compiler path:

It's simple. Quote from my prior comment:

#ifndef LLVM_USE_STATIC_ZSTD
#define LLVM_USE_STATIC_ZSTD 1
#endif

#if LLVM_USE_STATIC_ZSTD 
// Use static zstd
#else
// Use shared
#endif

Can we have something like this in sycl/CMakeList? For Fedora and other distros where you want to link against shared zstd library, you can just pass LLVM_USE_STATIC_ZSTD=0

[dvrogozh]

I'd like to highlight that install zstd on Windows and older RHEL OS is not trivial. User would have to build/download package and add to environment manually. The divergence from upstream LLVM is done solely for the convenience of our users.

What you describe is a standard situation in Linux ecosystem development when some new dependency is being introduced and adopted. Instead of swimming against the current and introduce non-standard development approaches it's much better to recognize that 1) there are already know ways to handle what you want to handle, 2) they exist for the reason, 3) they were introduced as a result of the actual experience many Linux developers in the community. I understand that on Windows it's a pain to build almost anything, but that's nether an excuse to step out from open source community practices. If you will look deeper you will find out that these practices do cover Windows case as well.

[uditagarwal97]

I'd like to highlight that install zstd on Windows and older RHEL OS is not trivial. User would have to build/download package and add to environment manually. The divergence from upstream LLVM is done solely for the convenience of our users.

What you describe is a standard situation in Linux ecosystem development when some new dependency is being introduced and adopted. Instead of swimming against the current and introduce non-standard development approaches it's much better to recognize that 1) there are already know ways to handle what you want to handle, 2) they exist for the reason, 3) they were introduced as a result of the actual experience many Linux developers in the community. I understand that on Windows it's a pain to build almost anything, but that's nether an excuse to step out from open source community practices. If you will look deeper you will find out that these practices do cover Windows case as well.

I understand your concern and if you feel strongly about making zstd a runtime dependency of DPCPP, let's start an internal discussion including SYCL architects and DPCPP component owners. But, we should definitely have a thorough discussion first before introducing a new runtime dependency to DPCPP. For the time being, to unblock Pytorch, can we use the workaround that I suggested? I am open to discussing other approaches as well.

[dvrogozh]

if you feel strongly about making zstd a runtime dependency of DPCPP, let's start an internal discussion including SYCL architects and DPCPP component owners.

I don't feel that this needs to be discussed. This sounds like bureaucracy discussion for no reason.

This is a strong requirement from Linux distros to support a build and runtime of the component (intel/llvm) against system installed dependencies. Note that a word "support" is important here - it means that this can be exposed as a choice to the user who will decide if he wants to have a runtime dependency or will prefer to link it in statically. The existing LLVM options already cover all these cases. In other words - this is not actually a component (DPCPP) call to use zstd as a static library, that's a user call. Note that if left unfixed this will be overridden by the patch carried on the Linux distro side.

[uditagarwal97]

if you feel strongly about making zstd a runtime dependency of DPCPP, let's start an internal discussion including SYCL architects and DPCPP component owners.

I don't feel that this needs to be discussed. This sounds like bureaucracy discussion for no reason.

I'm sorry if you feel that it's a bureaucratic discussion. Let me reiterate that it's done this way solely for the convenience of our users. If you have any alternative approach/ideas which solves your use case without being inconvenient to other users, I'm all ears.