This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ckissane on Jul 14 2022, 10:20 AM.

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
  • add a cmake macro (TryFindDependencyMacro.cmake) to aid in finding "modern cmake" style dependencies such as Zstd in an "optional" or "REQUIRED" manner.

Diff Detail

Event Timeline

ckissane created this revision.Jul 14 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 10:20 AM
ckissane requested review of this revision.Jul 14 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 10:20 AM
ckissane edited the summary of this revision. (Show Details)Jul 14 2022, 10:22 AM
ckissane updated this revision to Diff 444718.Jul 14 2022, 10:28 AM
  • add_library Zstd::zstd in FindZSTD.cmake
ckissane updated this revision to Diff 444836.EditedJul 14 2022, 4:26 PM
  • simplify zstd cmake code (by using "modern cmake" and no need for FindZSTD anymore)
ckissane updated this revision to Diff 444840.Jul 14 2022, 4:38 PM
  • make llvm-config --system-libs support output reflect zstd usage
ckissane updated this revision to Diff 444845.Jul 14 2022, 4:46 PM
  • remove get_library_name zstd because non standard
ckissane edited the summary of this revision. (Show Details)Jul 14 2022, 4:52 PM

It seems like FindZSTD.cmake went missing in the Diff 3 revision of this patch?

ckissane added a comment.EditedJul 15 2022, 11:18 AM

It seems like FindZSTD.cmake went missing in the Diff 3 revision of this patch?

@sebastian-n yes because facebook is surprisingly good at "modern cmake" practices with shipping libzstd and so `find_dependency(zstd)` is enough to have the lib `zstd::libzstd_shared``` get defined!
I have hopes that this will fix the issues mac os users reported with the old version of this patch.

ckissane updated this revision to Diff 445170.Jul 15 2022, 6:55 PM

make it try to find dependency instead of normal find_dependency macro (because the return breaks things on failure)

It seems like FindZSTD.cmake went missing in the Diff 3 revision of this patch?

@sebastian-n yes because facebook is surprisingly good at "modern cmake" practices with shipping libzstd and so `find_dependency(zstd)` is enough to have the lib `zstd::libzstd_shared``` get defined!
I have hopes that this will fix the issues mac os users reported with the old version of this patch.

Nice, makes sense. It definitely fixes the issue when llvm is included with add_subdirectory.

llvm/cmake/modules/LLVMConfig.cmake.in
79

Should this be lower case zstd? I saw some cmake errors here and they disappear when this is lower case.

ckissane updated this revision to Diff 445569.Jul 18 2022, 11:16 AM
  • move try_find_dependency into it's own file
ckissane added inline comments.Jul 18 2022, 11:19 AM
llvm/cmake/modules/LLVMConfig.cmake.in
79

good catch, It should also probably be another find_dependency call instead of find_package so I fixed that too

ckissane updated this revision to Diff 445570.Jul 18 2022, 11:22 AM
  • add newline to end of TryFindDependencyMacro.cmake
ckissane updated this revision to Diff 445571.Jul 18 2022, 11:26 AM
  • clarify comment in TryFindDependencyMacro.cmake
ckissane edited the summary of this revision. (Show Details)Jul 18 2022, 12:38 PM
ckissane abandoned this revision.Jul 18 2022, 1:43 PM

Abandoned because it duplicates https://reviews.llvm.org/D128465