Page MenuHomePhabricator

[llvm] add zstd to `llvm::compression` namespace
ClosedPublic

Authored by ckissane on Jun 23 2022, 2:27 PM.

Details

Summary
  • add zstd to llvm::compression namespace
  • add a CMake option LLVM_ENABLE_ZSTD with behavior mirroring that of LLVM_ENABLE_ZLIB
  • add tests for zstd to llvm/unittests/Support/CompressionTest.cpp
  • debian users should install libzstd when using LLVM_ENABLE_ZSTD=FORCE_ON from source due to this bug https://bugs.launchpad.net/ubuntu/+source/libzstd/+bug/1941956

Diff Detail

Unit TestsFailed

TimeTest
60,130 msx64 debian > BOLT.runtime/X86::exceptions-pic.test
Script: -- : 'RUN: at line 5'; /usr/bin/clang --driver-mode=g++ -fpic -Wl,-q /var/lib/buildkite-agent/builds/llvm-project/bolt/test/runtime/X86/Inputs/exception4.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/tools/bolt/test/runtime/X86/Output/exceptions-pic.test.tmp.pic
60,120 msx64 debian > Clang.Driver::emit-reproducer.c
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp
60,480 msx64 debian > Clang.Driver::fsanitize.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
60,160 msx64 debian > Clang.OpenMP::target_teams_distribute_parallel_for_simd_codegen_registration.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp
60,470 msx64 debian > Clang.OpenMP::target_update_codegen.cpp
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -DCK1 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp --check-prefix CK1 --check-prefix CK1-64
View Full Test Results (8 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ckissane updated this revision to Diff 444374.Jul 13 2022, 12:02 PM
  • Trim CMAKE modifications to minimum needed
ckissane marked an inline comment as done.Jul 13 2022, 12:02 PM

The patch looks mostly good but please fix the subject and the summary.

Note: if you fix the local commit message, you can use arc diff --head=HEAD 'HEAD^' --verbatim to update the subject and the summary.

llvm/lib/Support/Compression.cpp
83–86
130

const size_t

llvm/unittests/Support/CompressionTest.cpp
65

delete blank line

66

static void testZstd...

69

delete blank line

95
ckissane updated this revision to Diff 444400.Jul 13 2022, 1:07 PM

tidy some code

ckissane retitled this revision from [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace to [llvm] add zstd to `llvm::compression` namespace.Jul 13 2022, 1:09 PM
ckissane edited the summary of this revision. (Show Details)
ckissane marked 6 inline comments as done.Jul 13 2022, 1:13 PM

Mark handled comments from @MaskRay as done

phosek added inline comments.Jul 13 2022, 2:29 PM
llvm/lib/Support/Compression.cpp
62–63

I'd make changes to zlib separately.

83–86

Ditto here.

ckissane added inline comments.Jul 13 2022, 2:58 PM
llvm/lib/Support/Compression.cpp
62–63
ckissane updated this revision to Diff 444432.Jul 13 2022, 3:13 PM
  • Merge branch main
MaskRay accepted this revision.Jul 13 2022, 5:21 PM
MaskRay added inline comments.
llvm/include/llvm/Support/Compression.h
56

I changed the zlib functions to use uint8_t * and ArrayRef<uint8_t> instead. This patch needs to switch to uint8_t too.

llvm/lib/Support/Compression.cpp
154

uint8_t

This revision is now accepted and ready to land.Jul 13 2022, 5:21 PM
ckissane updated this revision to Diff 444489.Jul 13 2022, 7:04 PM
  • Merge remote-tracking branch 'origin/main' into ckissane.add-zstd
  • [Support] update zstd interface to use uint8_t * and ArrayRef<uint8_t>
This revision was landed with ongoing or failed builds.Jul 13 2022, 7:06 PM
This revision was automatically updated to reflect the committed changes.

Why was this reverted? Please make extensive tests especially for something dealing with the build system. When reverting a commit, briefly describe what happened.

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 a/llvm/cmake/modules/FindZSTD.cmake b/llvm/cmake/modules/FindZSTD.cmake
index 43ccf7232138..8e6ec6e8a988 100644
--- a/llvm/cmake/modules/FindZSTD.cmake
+++ b/llvm/cmake/modules/FindZSTD.cmake
@@ -12,6 +12,10 @@ find_package_handle_standard_args(
     ZSTD_LIBRARY ZSTD_INCLUDE_DIR)

 if(ZSTD_FOUND)
+    if(NOT TARGET Zstd::zstd)
+      add_library(Zstd::zstd UNKNOWN IMPORTED)
+      set_target_properties(Zstd::zstd PROPERTIES IMPORTED_LOCATION "${ZSTD_LIBRARY}")
+    endif()
     set(ZSTD_LIBRARIES ${ZSTD_LIBRARY})
     set(ZSTD_INCLUDE_DIRS ${ZSTD_INCLUDE_DIR})
 endif()
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index 52b95c5377d3..031adfa33ba8 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -26,7 +26,7 @@ if(LLVM_ENABLE_ZLIB)
 endif()

 if(LLVM_ENABLE_ZSTD)
-  list(APPEND imported_libs zstd)
+  list(APPEND imported_libs Zstd::zstd)
 endif()

 if( MSVC OR MINGW )

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 <zstd.h>
         ^~~~~~~~

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 <zstd.h>
         ^~~~~~~~

@aemerson Could you provide the output of your cmake command?
(I can't easily reproduce because I don't have a mac on hand)

@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.

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 <zstd.h>
         ^~~~~~~~

@aemerson Could you provide the output of your cmake command?
(I can't easily reproduce because I don't have a mac on hand)

FWIW I hit this last night on my mac desktop - it looked like I had zstd installed by homebrew (as a dependency on something I installed), and cmake was able to find it in /opt/homebrew (somehow) but when Compression.cpp was built, nothing had added -I/opt/homebrew/include so the header wasn't found. I hacked the FindZSTD.cmake to not find it, to finish what I was working on before bedtime. This might not be related to what @aemerson saw though.

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 <zstd.h>
         ^~~~~~~~

@aemerson Could you provide the output of your cmake command?
(I can't easily reproduce because I don't have a mac on hand)

FWIW I hit this last night on my mac desktop - it looked like I had zstd installed by homebrew (as a dependency on something I installed), and cmake was able to find it in /opt/homebrew (somehow) but when Compression.cpp was built, nothing had added -I/opt/homebrew/include so the header wasn't found. I hacked the FindZSTD.cmake to not find it, to finish what I was working on before bedtime. This might not be related to what @aemerson saw though.

@jasonmolenda Thank you for the info, I believe it is similar, could you try: https://reviews.llvm.org/D129786 for me?

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 <zstd.h>
         ^~~~~~~~

@aemerson Could you provide the output of your cmake command?
(I can't easily reproduce because I don't have a mac on hand)

FWIW I hit this last night on my mac desktop - it looked like I had zstd installed by homebrew (as a dependency on something I installed), and cmake was able to find it in /opt/homebrew (somehow) but when Compression.cpp was built, nothing had added -I/opt/homebrew/include so the header wasn't found. I hacked the FindZSTD.cmake to not find it, to finish what I was working on before bedtime. This might not be related to what @aemerson saw though.

@jasonmolenda Thank you for the info, I believe it is similar, could you try: https://reviews.llvm.org/D129786 for me?

I applied the patch from https://reviews.llvm.org/D129786 on a clean checkout, cmake ../llvm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=1 -DLLVM_ENABLE_PROJECTS='llvm;lldb;clang' '-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi' -DLLDB_ENABLE_PYTHON=1 (my normal cmake line) and got

-- Found ZSTD: /opt/homebrew/lib/libzstd.dylib  
-- Looking for ZSTD_compress
-- Looking for ZSTD_compress - found

[...]

FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/Compression.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/jason/k/llvm/llvm-project/build/lib/Support -I/Users/jason/k/llvm/llvm-project/llvm/lib/Support -I/Users/jason/k/llvm/llvm-project/build/include -I/Users/jason/k/llvm/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color  -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk -std=c++14  -fno-exceptions -fno-rtti -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/Compression.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/Compression.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/Compression.cpp.o -c /Users/jason/k/llvm/llvm-project/llvm/lib/Support/Compression.cpp
/Users/jason/k/llvm/llvm-project/llvm/lib/Support/Compression.cpp:24:10: fatal error: 'zstd.h' file not found
#include <zstd.h>
         ^~~~~~~~
1 error generated.

% file /opt/homebrew/lib/libzstd.dylib 
/opt/homebrew/lib/libzstd.dylib: Mach-O 64-bit dynamically linked shared library arm64
% head -2 /opt/homebrew/include/zstd.h 
/*
 * Copyright (c) Yann Collet, Facebook, Inc.
%

% grep -i zstd CMakeCache.txt 
//Use zstd for compression/decompression if available. Can be ON,
LLVM_ENABLE_ZSTD:STRING=ON
ZSTD_INCLUDE_DIR:PATH=/opt/homebrew/include
ZSTD_LIBRARY:FILEPATH=/opt/homebrew/lib/libzstd.dylib
//Details about finding ZSTD
FIND_PACKAGE_MESSAGE_DETAILS_ZSTD:INTERNAL=[/opt/homebrew/lib/libzstd.dylib][/opt/homebrew/include][v()]
//Have symbol ZSTD_compress
HAVE_ZSTD:INTERNAL=1
//ADVANCED property for variable: ZSTD_INCLUDE_DIR
ZSTD_INCLUDE_DIR-ADVANCED:INTERNAL=1
//ADVANCED property for variable: ZSTD_LIBRARY
ZSTD_LIBRARY-ADVANCED:INTERNAL=1
MaskRay reopened this revision.Jul 17 2022, 10:42 AM
This revision is now accepted and ready to land.Jul 17 2022, 10:42 AM
ckissane retitled this revision from [llvm] add zstd to `llvm::compression` namespace to [OLD] [llvm] add zstd to `llvm::compression` namespace.Jul 18 2022, 12:27 PM
ckissane edited the summary of this revision. (Show Details)
ckissane edited the summary of this revision. (Show Details)

Instead of marking a differential [OLD] (which may confuse readers whether this is to be abandoned), you can mark a differential Request Reviews/Request Changes. Then it will not be in the green state.

I clicked "Reopen" to make it clear the patch hasn't relanded.

Instead of marking a differential [OLD] (which may confuse readers whether this is to be abandoned), you can mark a differential Request Reviews/Request Changes. Then it will not be in the green state.

I clicked "Reopen" to make it clear the patch hasn't relanded.

Oh, sorry about that!
I had actually created https://reviews.llvm.org/D129786 instead of reopening this one because at the time I tried, reopening didn't seem to work.

MaskRay added a comment.EditedJul 18 2022, 12:40 PM

Instead of marking a differential [OLD] (which may confuse readers whether this is to be abandoned), you can mark a differential Request Reviews/Request Changes. Then it will not be in the green state.

I clicked "Reopen" to make it clear the patch hasn't relanded.

Oh, sorry about that!

That's fine :) We need to learn the web UI.

I had actually created https://reviews.llvm.org/D129786 instead of reopening this one because at the time I tried, reopening didn't seem to work.

A reverted patch by default is still in the "Closed" state. You can "reopen" it so that people know it hasn't relanded.
When repairing a differential, just reuse the existing patch. It keeps all discussions (including past suggestions and issues a previous commit did not address) in one thread.
If the new patch is very similar to this one, you probably need to abandon it in favor of this one.


If you revert B because you haven't done A, and A and B are somewhat not so related (e.g. B addresses an existing brittle build system issue which happens to affect some platform when A is added), then having A as a separate differential is fine.

ckissane retitled this revision from [OLD] [llvm] add zstd to `llvm::compression` namespace to [llvm] add zstd to `llvm::compression` namespace.Jul 18 2022, 1:42 PM
ckissane edited the summary of this revision. (Show Details)
ckissane updated this revision to Diff 445615.Jul 18 2022, 1:42 PM
  • move try_find_dependency into it's own file
  • add newline to end of TryFindDependencyMacro.cmake
  • clarify comment in TryFindDependencyMacro.cmake
ckissane updated this revision to Diff 445623.Jul 18 2022, 2:14 PM
  • remove extra cmake info
ckissane edited the summary of this revision. (Show Details)Jul 18 2022, 2:15 PM
ckissane requested review of this revision.Jul 18 2022, 2:56 PM
MaskRay accepted this revision.Jul 18 2022, 3:00 PM

Does this address the macOS build failure?

This revision is now accepted and ready to land.Jul 18 2022, 3:00 PM
ckissane edited the summary of this revision. (Show Details)Jul 18 2022, 3:24 PM
ckissane edited the summary of this revision. (Show Details)
ckissane added a comment.EditedJul 18 2022, 3:26 PM

Does this address the macOS build failure?

I believe so! @leonardchan is double checking

leonardchan accepted this revision.Jul 19 2022, 10:52 AM

LGTM tested on mac on my end

This revision was landed with ongoing or failed builds.Jul 19 2022, 10:54 AM
This revision was automatically updated to reflect the committed changes.

@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 CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "zstd", but
  CMake did not find one.

  Could not find a package configuration file provided by "zstd" with any of
  the following names:

    zstdConfig.cmake
    zstd-config.cmake

  Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
  "zstd_DIR" to a directory containing one of the above files.  If "zstd"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CMakeLists.txt:768 (include)

Ubuntu 22.04 LTS
cmake version 3.22.3

phosek added inline comments.Jul 20 2022, 12:33 AM
llvm/cmake/config-ix.cmake
149

Since there's no Findzstd.cmake module shipped with CMake, and we don't provide in LLVM, we should use the config mode.

I am still seeing the

zstdConfig.cmake
zstd-config.cmake

warning. @ckissane @phosek will you fix it? :)

I am still seeing the

zstdConfig.cmake
zstd-config.cmake

warning. @ckissane @phosek will you fix it? :)

I've quieted the warnings in https://github.com/llvm/llvm-project/commit/a71a01bc10d509bd4e746e7d277c4613ef886875, though I'm just now seeing @phosek's suggestion to use CONFIG I'm not sure what that does

MaskRay added inline comments.Jul 25 2022, 10:34 AM
llvm/include/llvm/Support/Compression.h
49

I missed the values here. Why is -5 picked for NoCompression? What does it mean?

zstd.h says ZSTD_maxCLevel() is currently 22. The CLI program mentions 19. Why is BestSizeCompression 12?

ZSTD_CLEVEL_DEFAULT/the CLI CLI uses 3 for the default level. Why is DefaultCompression 5?

MaskRay added inline comments.Wed, Sep 14, 10:56 AM
llvm/include/llvm/Support/Compression.h
49

Ping @ckissane about the choice.

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 cmake/config-ix.cmake:144 (find_package):
  By not providing "Findzstd.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "zstd", but
  CMake did not find one.

  Could not find a package configuration file provided by "zstd" with any of
  the following names:

    zstdConfig.cmake
    zstd-config.cmake

  Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
  "zstd_DIR" to a directory containing one of the above files.  If "zstd"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CMakeLists.txt:774 (include)

It is impossible to satisfy this dependency when bootstrapping a static build of zig without patching LLVM.

MaskRay added a comment.EditedMon, Sep 19, 12:23 PM

zstd provides GNU Makefile, CMake, and Meson. The CMake files are only installed when configured with CMake. Debian and Ubuntu lack them.
The pkg-config file libzstd.pc probably provides the best portability on at least *NIX systems. (I have also used it for a binutils-gdb patch.)

I think we can then avoid the

zstdConfig.cmake
zstd-config.cmake

issue.

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 standard CMAKE_PREFIX_PATH.

If LLVM will not carry Findzstd.cmake then it should not use FIND_PACKAGE in the case of -DLLVM_ENABLE_ZSTD=FORCE_ON; it should just throw -lzstd on the linker line instead of throwing rakes in my path for no reason.

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 addressed by D132870 and D133222.

Also LLVM needs to now include a Findzstd.cmake file or else we get this error:

CMake Error at cmake/config-ix.cmake:144 (find_package):
  By not providing "Findzstd.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "zstd", but
  CMake did not find one.

  Could not find a package configuration file provided by "zstd" with any of
  the following names:

    zstdConfig.cmake
    zstd-config.cmake

  Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
  "zstd_DIR" to a directory containing one of the above files.  If "zstd"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CMakeLists.txt:774 (include)

It is impossible to satisfy this dependency when bootstrapping a static build of zig without patching LLVM.

It should be possible to use CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON to pick up config file that's part of the zstd installation, but I'd be also fine including Findzstd.cmake in LLVM to avoid needing that.

Is anyone working on a solution that would work for people using a zstd install method that's actually supported upstream?

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 be great if others could test it on their system.