[SYCL][Clang] Fix compilation with --offload-compress and missing zstd

[ph0b]

When using a compiler built without zstd support, it's preferable for --offload-compress option to lead to a warning instead of an error.

[uditagarwal97]

When using a compiler built without zstd support, it's preferable for --offload-compress option to lead to a warning instead of an error.

Sorry, what's the motivation behind this change?
The reason we throw an error is because:
(1) That's what LLVM upstream do in clang-offload-bundler, when --offload-compress is passed but zstd is not available. See #15124 (comment)
(2) From user's POV, if the user specifies --offload-compress it is expected that the compiler will compress device images. If the compiler is build w/o zstd support and images can't be compressed, we should alert the user about it, instead of silently producing a warning and ignoring --offload-compress.

Also, AFAIK, we already build compiler with zstd for both opensource and OneAPI releases, so, not sure about the problem you're trying to solve.

[ph0b]

Sorry, what's the motivation behind this change?

I've encountered this issue after recompiling Blender with my own build of DPC++ with Nvidia and AMD support on Windows (something that's not offered by open-source nor proprietary releases on Windows).

The default build settings of DPC++ enable ZSTD with just ON, not FORCE_ON, so I had no reported failure when building the compiler itself but ended up with a version without ZSTD support.
However, when trying to use this compiler with Blender, it couldn't compile it out of the box, because Blender sets --offload-compress.

From an user point of view, it was surprising to get an error at this stage, a warning would have made more sense, as offload compression not being available is not going to prevent correct compilation and execution. The produced binaries will just be bigger than expected, for which a warning would be enough. A simple switch of compiler should not require to change hardcoded compilation flags.

I don't know the use cases for clang-offload-bundler so I can't tell if this upstream behavior is correct, but in the higher level clang++ -fsycl --offload-compress case, my take is that a compiler without zstd support should still generate usable binaries, with a warning that offload-compression has no effect.

If you'd rather not fix it this way, another solution is to switch the default setting for ZSTD to FORCE_ON to make zstd a hard requirement.

[uditagarwal97]

However, when trying to use this compiler with Blender, it couldn't compile it out of the box, because Blender sets --offload-compress.

That sounds like a Blender-side issue to me. If Blender is setting --offload-compress, it is assuming that the compiler is built with zstd support. Why don't we change Blender's cmake files to make sure that we only set --offload-compress when DPC++ is built with zstd? Something like what we do in our LIT config files: https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/lit.cfg.py#L387

If you'd rather not fix it this way, another solution is to switch the default setting for ZSTD to FORCE_ON to make zstd a hard requirement.

We can not change to FORCE_ON because internally, not all of our development/testing machines has zstd installed. Setting FORCE_ON would break DPCPP build when zstd is not present.

[ph0b]

If Blender is setting --offload-compress, it is assuming that the compiler is built with zstd support.

Blender is assuming recent compiler versions offer the option and the associated documentation

llvm/sycl/doc/UsersManual.md

Line 203 in 8ced24f

**`--offload-compress`**

doesn't specify that compiler builds can fail when using such flag when zstd support isn't built-in.

When using that compiler flag with older compiler versions not supporting the feature, we only get a warning:
clang++: warning: argument unused during compilation: '--offload-compress'

Why don't we change Blender's cmake files to make sure that we only set --offload-compress when DPC++ is built with zstd?

Compiling a blank program with --offload-compress and -Werror to check the feature works is doable but it's quite overkill compared to the warning I would expect. If it must remain an error from your point of view, I'll see this being more acceptable if you can clarify the documentation on the expected behavior and need for testing before using such flag.

not all of our development/testing machines has zstd installed

Only development machines should need zstd, and that's the goal if building only versions that provide device images compression. Testing machines shouldn't have more installed than OS+GPU drivers.

[uditagarwal97]

When using that compiler flag with older compiler versions not supporting the feature, we only get a warning: clang++: warning: argument unused during compilation: '--offload-compress'

That's because --offload-compress option is also used by HIP to compress device code IR while bundling. And that's why, when offloading to SYCL, --offload-compress option is ignored by the clang driver, for older DPC++ versions only. Even for HIP, when you build the compiler without zstd & libz and use --offload-compress option, it throws

If it must remain an error from your point of view, I'll see this being more acceptable if you can clarify the documentation on the expected behavior and need for testing before using such flag.

IMO, the current behavior is correct: there should be an error when specifying --offload-compress CLI option for SYCL offloading but compiler is not built with zstd. @mdtoguchi (@intel/dpcpp-clang-driver-reviewers) thoughts?
Regarding the documentation, totally, I could clarify the documentation of --offload-compress to document this behavior.

[mdtoguchi]

IMO, the current behavior is correct: there should be an error when specifying --offload-compress CLI option for SYCL offloading but compiler is not built with zstd. @mdtoguchi (@intel/dpcpp-clang-driver-reviewers) thoughts? Regarding the documentation, totally, I could clarify the documentation of --offload-compress to document this behavior.

I would agree, we should keep the current behavior and emit the error. Updating documentation to reflect the zstd requirement will definitely help any confusion moving forward. It should be the responsibility of who is building and providing the compiler to make sure required dependencies are met.

[ph0b]

Superseded by #17957

[sherholz-intel]

@uditagarwal97 and @mdtoguchi when updating the documentation I think it would be important to also provide a CMake macro as example (in the documentation) which can just be re-used by the users/developers when they want to add --offload-compress to their project.

In #17957 I posted my experience with the current error message and from a user perspective it could happen quite easily that the user thinks the compiler or this feature is broken.
A reason for that is that the error complains that zstd is not found/installed even if it is installed on the system.
It does not say the compiler was not build with this support (i.e., libzstd-dev was not installed on the build system of the compiler).

[uditagarwal97]

@uditagarwal97 and @mdtoguchi when updating the documentation I think it would be important to also provide a CMake macro as example (in the documentation) which can just be re-used by the users/developers when they want to add --offload-compress to their project.

In #17957 I posted my experience with the current error message and from a user perspective it could happen quite easily that the user thinks the compiler or this feature is broken. A reason for that is that the error complains that zstd is not found/installed even if it is installed on the system. It does not say the compiler was not build with this support (i.e., libzstd-dev was not installed on the build system of the compiler).

I've improved the error message in #17990