-
Notifications
You must be signed in to change notification settings - Fork 769
[Clang][Doc][NFC] Improve -offload-compress
documentation and error message
#17990
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
Conversation
zstd
and -offload-compress
documentation and error message-offload-compress
documentation and error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm just nits
sycl/doc/GetStartedGuide.md
Outdated
```sh | ||
sudo apt-get install libzstd-dev | ||
``` | ||
Note that the libzstd-dev package provided on Ubuntu 24.04 has a bug and the zstd static library is not built with `-fPIC` flag. Linking to this library will result in a build failure. For example: [Issue#15935](https://github.com/intel/llvm/issues/15935). As an alternative, zstd can be built from source either manually or by using the [build_zstd_1_5_6_ub24.sh](https://github.com/intel/llvm/blob/sycl/devops/scripts/build_zstd_1_5_6_ub24.sh) script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be helpful to link to the launchpad bug here so people can know when its fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Done in d498a14
@@ -134,6 +134,9 @@ def do_configure(args, passthrough_args): | |||
if args.use_lld: | |||
llvm_enable_lld = "ON" | |||
|
|||
if args.use_zstd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to use this in ci_defaults
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but let's do that change in a separate PR. I am not sure if the new Windows machine that we recently added (on VM) has zstd
installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i should have installed it everywhere but yeah a separate pr is fine
@steffenlarsen ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good, but I have a couple small comments.
Co-authored-by: Steffen Larsen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Based on the user's feedback in #17957 and #17914