[llvm] Remove unused and redundant crc32 funcction from llvm::compression::zlib namespace

[ckissane]

Summary

• Remove crc32 from zlib compression namespace, people should use the llvm::crc32 instead.

[ckissane]

up release notes

[leonardchan]

Feedback needed on:
this namespace approach will result in tooling compression methods being set at compile time, however we may want to aim for a runtime configurable approach in the future, likely a new revision of the compressed data formats that llvm interfaces with with outside tools IE:

• clang ast serialization code could be changed so compressed data could have a flag indicating a compression type.
• profile data code could be changed so compressed data could have a flag indicating a compression type as well.

Using the compile time approach for now with the runtime approach in the future SGTM. I think some flag that could specify the decompression type is what we want in the long term, but the compile time approach I think is ok for now to not block you.

clang-tools-extra/clangd/index/Serialization.cpp

242 ↗

(On Diff #440736)

nit: Could we add some function that returns a string of whatever compression is used? This way we have a more descriptive error message showing what specifically is unavailable. Same comment elsewhere there's logging/error reporting but the string is "compression::serialize".

llvm/unittests/Support/CompressionTest.cpp

70

Should this be replaced with llvm::crc32 rather than deleting this?

[ckissane]

llvm/unittests/Support/CompressionTest.cpp

70

No Because llvm::crc32 has it's own tests already

[ckissane]

clang-tools-extra/clangd/index/Serialization.cpp

242 ↗

(On Diff #440736)

sure, I can add a AlgorithmName property to the zlib namespace and future compression namespaces, then we can print out compression::serialize::AlgorithmName to see what is being used

[ckissane]

add AlgorithmName to compression namespaces

[ckissane]

Marked name of algorithm comments as done

[ckissane]

• feat: refactor llvm compression namespaces
• chore: update rel notes to note zlib::crc32 remove
• feat: add AlgorithmName to compression namespaces

[ckissane]

• Merge remote-tracking branch 'origin/main' into ckissane.refactor-compression-namespace

[leonardchan]

llvm/docs/ReleaseNotes.rst

183–191

I think maybe even more of these could be split into further patches. I would expect that this patch contain just the namespace refactoring but not necessarily any code deletion or cmake changes. I think this could be:

• One patch that's just the namespace changes; anything else like variable name changes or string changes would be in separate patches, then this patch would be pure RFC and can get a quick LGTM
  • One followup patch to this that adds AlgorithmName to the nested zlib namespace.
• One patch that removes crc32 (and its test)
• One patch for cmake changes
• One patch for the zlib_unavailable -> compression_unavailable change and logging string changes in profdata

And if necessary, each of them would have an appropriate ReleaseNodes.rst update.

llvm/lib/Support/Compression.cpp

30

Should createError still be wrapped with #if LLVM_ENABLE_ZLIB?

[MaskRay]

llvm/docs/ReleaseNotes.rst

183–191

I agree with this plan. Changes like zlib_unavailable need to bring attention to the usual contributors/reviewers in the area.

llvm/lib/Support/Compression.cpp

28

llvm/unittests/Support/CompressionTest.cpp

22

[ckissane]

make part 1 after part 0 of refactor

[ckissane]

• Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-1

[leonardchan]

LGTM. Should probably add [llvm] to the subject also. Generally it's helpful for immediately knowing which subrepo you're touching.

[ckissane]

addressed comments marked done

[ckissane]

updated diff

[MaskRay]

Thanks!

llvm/lib/Support/Compression.cpp

25

if a previous patch introduces a blank line between two using. please drop it from the previous patch.

llvm/unittests/Support/CompressionTest.cpp

21

if a previous patch introduces a blank line between two using. please drop it from the previous patch.

[ckissane]

• remove crc32 from zlib compression namespace
• Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-1
• Merge branch 'ckissane.refactor-compression.part-0.no-string-sub' into ckissane.refactor-compression.part-1
• Merge remote-tracking branch 'origin/ckissane.refactor-compression.part-0.no-string-sub' into ckissane.refactor-compression.part-1

[ckissane]

mark handled comments as done

[MaskRay]

You may push this patch and its prerequisite while other patches are still to be reviewed.

llvm/docs/ReleaseNotes.rst

* Add ``LLVMGetAggregateElement`` function as a wrapper for
   only instances of ``ConstantDataSequential``.
 
 * The following functions for creating constant expressions have been removed,
   because the underlying constant expressions are no longer supported. Instead,
   an instruction should be created using the ``LLVMBuildXYZ`` APIs, which will
   constant fold the operands if possible and create an instruction otherwise:
   * ``LLVMConstExtractValue``
 
 * Refactor compression namespaces across the project, making way for a possible
   introduction of alternatives to zlib compression in the llvm toolchain.
   Changes are as follows:
   * Relocate the ``llvm::zlib`` namespace to ``llvm::compression::zlib``.
+  * Remove crc32 from zlib compression namespace, people should use the ``llvm::crc32`` instead.
 
 Changes to the Go bindings
 --------------------------
 
 
 Changes to the FastISel infrastructure
 --------------------------------------
 
 * ...
 
 Changes to the DAG infrastructure
 ---------------------------------

llvm/include/llvm/Support/Compression.h

 
 Error uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                  size_t &UncompressedSize);
 
 Error uncompress(StringRef InputBuffer,
                  SmallVectorImpl<char> &UncompressedBuffer,
                  size_t UncompressedSize);
 
-uint32_t crc32(StringRef Buffer);
-
 } // End of namespace zlib
 
 } // End of namespace compression
 
 } // End of namespace llvm
 
 #endif

llvm/lib/Support/Compression.cpp

 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #if LLVM_ENABLE_ZLIB
 #include <zlib.h>
 #endif
 
 using namespace llvm;
 
 using namespace llvm::compression;
 
 #if LLVM_ENABLE_ZLIB
 
 static StringRef convertZlibCodeToString(int Code) {
   switch (Code) {
   case Z_MEM_ERROR:
     return "zlib error: Z_MEM_ERROR";
   case Z_BUF_ERROR:
     return "zlib error: Z_BUF_ERROR";
   case Z_STREAM_ERROR:
     return "zlib error: Z_STREAM_ERROR";
   case Z_DATA_ERROR:
Error zlib::uncompress(StringRef InputBuffer,
                        size_t UncompressedSize) {
   UncompressedBuffer.resize_for_overwrite(UncompressedSize);
   Error E = zlib::uncompress(InputBuffer, UncompressedBuffer.data(),
                              UncompressedSize);
   UncompressedBuffer.truncate(UncompressedSize);
   return E;
 }
 
-uint32_t zlib::crc32(StringRef Buffer) {
-  return ::crc32(0, (const Bytef *)Buffer.data(), Buffer.size());
-}
-
 #else
 bool zlib::isAvailable() { return false; }
 void zlib::compress(StringRef InputBuffer,
                     SmallVectorImpl<char> &CompressedBuffer, int Level) {
   llvm_unreachable("zlib::compress is unavailable");
 }
 Error zlib::uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                        size_t &UncompressedSize) {
   llvm_unreachable("zlib::uncompress is unavailable");
 }
 Error zlib::uncompress(StringRef InputBuffer,
                        SmallVectorImpl<char> &UncompressedBuffer,
                        size_t UncompressedSize) {
   llvm_unreachable("zlib::uncompress is unavailable");
 }
-uint32_t zlib::crc32(StringRef Buffer) {
-  llvm_unreachable("zlib::crc32 is unavailable");
-}
 #endif

llvm/unittests/Support/CompressionTest.cpp

 #include "llvm/Support/Compression.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 using namespace llvm::compression;
 
 namespace {
 
 #if LLVM_ENABLE_ZLIB
 
 void TestZlibCompression(StringRef Input, int Level) {
   SmallString<32> Compressed;
   SmallString<32> Uncompressed;
TEST(CompressionTest, Zlib) {
   StringRef BinaryDataStr(BinaryData, kSize);
 
   TestZlibCompression(BinaryDataStr, zlib::NoCompression);
   TestZlibCompression(BinaryDataStr, zlib::BestSizeCompression);
   TestZlibCompression(BinaryDataStr, zlib::BestSpeedCompression);
   TestZlibCompression(BinaryDataStr, zlib::DefaultCompression);
 }
 
-TEST(CompressionTest, ZlibCRC32) {
-  EXPECT_EQ(
-      0x414FA339U,
-      zlib::crc32(StringRef("The quick brown fox jumps over the lazy dog")));
-}
-
 #endif
 
 }