[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

aaronmondal added a comment. Porting to bazel in https://reviews.llvm.org/D143344 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 ___ cfe-commits mailing list

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

phosek added a comment. In D128465#3826150 , @mgorny wrote: > Is anyone working on a solution that would work for people using a zstd > install method that's actually supported upstream? I have created D134990 , it'd

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

mgorny added a comment. Is anyone working on a solution that would work for people using a zstd install method that's actually supported upstream? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

phosek added a comment. In D128465#3800479 , @andrewrk wrote: > if(LLVM_ENABLE_ZSTD) > list(APPEND imported_libs zstd::libzstd_shared) > endif() > > This hard codes shared linking which breaks the use case of static linking > LLVM. This was

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

andrewrk added a comment. Compiler infrastructure should not assume the existence of a Linux distribution. The portable way is simple and can easily be supported. One should be able to install `$prefix/lib/libzstd.a` and `$prefix/include/zstd.h`, then have that prefix searched as part of the

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. zstd provides GNU Makefile, CMake, and Meson. The CMake files are only installed when configured with CMake. Debian and Ubuntu lack such files. The pkg-config file libzstd.pc is probably the most portable file. (I have also used it for a binutils-gdb patch.) I think we

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

andrewrk added a comment. if(LLVM_ENABLE_ZSTD) list(APPEND imported_libs zstd::libzstd_shared) endif() This hard codes shared linking which breaks the use case of static linking LLVM. Also LLVM needs to now include a Findzstd.cmake file or else we get this error: CMake Error at

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; MaskRay wrote: > I missed the values here. Why is -5 picked for NoCompression? What does it >

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; I missed the values here. Why is -5 picked for NoCompression? What does it mean? zstd.h says

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

abrachet added a comment. In D128465#3672515 , @MaskRay wrote: > I am still seeing the > > zstdConfig.cmake > zstd-config.cmake > > warning. @ckissane @phosek will you fix it? :) I've quieted the warnings in

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. I am still seeing the zstdConfig.cmake zstd-config.cmake warning. @ckissane @phosek will you fix it? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

phosek added inline comments. Comment at: llvm/cmake/config-ix.cmake:149 + elseif(NOT LLVM_USE_SANITIZER MATCHES "Memory.*") +find_package(zstd) + endif() Since there's no `Findzstd.cmake` module shipped with CMake, and we don't provide in LLVM, we should

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

barannikov88 added a comment. @ckissane I took fresh sources of LLVM and when I run cmake I get the following warning: -- Looking for compress2 -- Looking for compress2 - found CMake Warning at cmake/config-ix.cmake:149 (find_package): By not providing "Findzstd.cmake" in

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe939bf67e340: [llvm] add zstd to `llvm::compression` namespace (authored by ckissane). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

leonardchan accepted this revision. leonardchan added a comment. LGTM tested on mac on my end Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 ___ cfe-commits

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane added a comment. In D128465#3661049 , @MaskRay wrote: > Does this address the macOS build failure? I believe so! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Does this address the macOS build failure? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane updated this revision to Diff 445623. ckissane added a comment. - remove extra cmake info Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 Files: llvm/CMakeLists.txt

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane updated this revision to Diff 445615. ckissane added a comment. - move try_find_dependency into it's own file - add newline to end of TryFindDependencyMacro.cmake - clarify comment in TryFindDependencyMacro.cmake Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

jasonmolenda added a comment. In D128465#3653216 , @ckissane wrote: > In D128465#3653051 , @jasonmolenda > wrote: > >> In D128465#3652525 , @ckissane >> wrote: >> >>>

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane added a comment. In D128465#3653051 , @jasonmolenda wrote: > In D128465#3652525 , @ckissane > wrote: > >> In D128465#3651025 , @aemerson >> wrote: >> >>> I

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

jasonmolenda added a comment. In D128465#3652525 , @ckissane wrote: > In D128465#3651025 , @aemerson > wrote: > >> I just reverted this in 6e6be5f9504d >>

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane added a comment. @aemerson can you let me know if https://reviews.llvm.org/D129786 works on MacOS? I am continuing work on that revision because this one has a lot of outdated unrelated history. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane added a comment. In D128465#3651025 , @aemerson wrote: > I just reverted this in 6e6be5f9504d > because > it seems to have broken macOS builds: > >

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

aemerson added a comment. I just reverted this in 6e6be5f9504d because it seems to have broken macOS builds: llvm/lib/Support/Compression.cpp:24:10: fatal error: 'zstd.h' file not found #include ^~~~

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

sebastian-ne added a comment. This breaks builds when LLVM is included with CMake’s `add_subdirectory`. I think the zstd target needs to be marked as imported. The following patch fixes the problem, although I’m not familiar enough with CMake to know if this is the right way to go: diff --git

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay added a comment. Why was this reverted? Please make extensive tests especially for something dealing with the build system. When reverting a commit, briefly describe what happened. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcef07169ec9f: [llvm] add zstd to `llvm::compression` namespace (authored by ckissane). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane updated this revision to Diff 89. ckissane added a comment. - Merge remote-tracking branch 'origin/main' into ckissane.add-zstd - [Support] update zstd interface to use uint8_t * and ArrayRef Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: llvm/include/llvm/Support/Compression.h:54 + +void compress(StringRef InputBuffer, SmallVectorImpl , + int Level = DefaultCompression);

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane updated this revision to Diff 32. ckissane added a comment. - Merge branch main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 Files: llvm/CMakeLists.txt llvm/cmake/config-ix.cmake

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane added inline comments. Comment at: llvm/lib/Support/Compression.cpp:63 __msan_unpoison(CompressedBuffer.data(), CompressedSize); - CompressedBuffer.truncate(CompressedSize); + if (CompressedSize < CompressedBuffer.size()) +

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

phosek added inline comments. Comment at: llvm/lib/Support/Compression.cpp:63 __msan_unpoison(CompressedBuffer.data(), CompressedSize); - CompressedBuffer.truncate(CompressedSize); + if (CompressedSize < CompressedBuffer.size()) +

[PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

ckissane marked 6 inline comments as done. ckissane added a comment. Mark handled comments from @MaskRay as done Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465