Page MenuHomePhabricator

[NFC] Refactor llvm::zlib namespace
ClosedPublic

Authored by ckissane on Jun 30 2022, 3:07 PM.

Details

Summary
  • Refactor compression namespaces across the project, making way for a possible introduction of alternatives to zlib compression. Changes are as follows:
    • Relocate the llvm::zlib namespace to llvm::compression::zlib.

Diff Detail

Event Timeline

ckissane created this revision.Jun 30 2022, 3:07 PM
ckissane requested review of this revision.Jun 30 2022, 3:07 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 30 2022, 3:07 PM
ckissane retitled this revision from chore: refactor compression namespaces 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`/ * Code... to chore: refactor compression namespaces making way for a possibleintroduction of alternatives to zlib compression in the llvm toolchain..Jun 30 2022, 3:09 PM
ckissane edited the summary of this revision. (Show Details)
ckissane added a reviewer: leonardchan.
ckissane retitled this revision from chore: refactor compression namespaces making way for a possibleintroduction of alternatives to zlib compression in the llvm toolchain. to refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain..
leonardchan accepted this revision.Jun 30 2022, 3:19 PM

LGTM. Should also probably add [NFC] to the start of the subject

lld/ELF/InputSection.cpp
79

": contains a compressed section, " + "but " -> ": contains a compressed section, but "

llvm/lib/ProfileData/InstrProf.cpp
155

" compression but the profile reader was built " + "without " -> " compression but the profile reader was built without "

This revision is now accepted and ready to land.Jun 30 2022, 3:19 PM
ckissane retitled this revision from refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain. to [NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain..Jun 30 2022, 3:21 PM
ckissane updated this revision to Diff 441540.Jun 30 2022, 3:22 PM
  • Merge branch 'ckissane.refactor-compression.part-0' of github.com:ckissane/llvm-project into ckissane.refactor-compression.part-0
  • Merge branch 'main' into ckissane.refactor-compression.part-0
phosek added a subscriber: phosek.Jun 30 2022, 11:51 PM
phosek added inline comments.
llvm/include/llvm/Support/Compression.h
49–51

I think we will need to support dynamically selecting (de)compression algorithm for both profile and serialization. For example, we should be able to use read profiles generated by an older version of LLVM that only supported zlib, even if the new one also supports zstd. Given that, I'd omit these and instead use compression::zlib everywhere.

[NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain.

I'd use [NFC] Refactor llvm::zlib namespace and place supplementary wording to the body.

in the llvm toolchain is redundant in the sentence.

MaskRay added inline comments.Jul 1 2022, 10:42 AM
llvm/lib/ProfileData/InstrProf.cpp
155

Keep the diagnostic unchanged in this patch.

ckissane retitled this revision from [NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain. to [NFC] Refactor llvm::zlib namespace.Jul 1 2022, 11:01 AM
ckissane edited the summary of this revision. (Show Details)
ckissane added inline comments.Jul 1 2022, 11:20 AM
llvm/include/llvm/Support/Compression.h
49–51

Correct, however I think that doing this in the meantime helps make it clear semantically for what purpose each compression call is for. It will make finding all instances of each time of use much easier in the future, and promotes semantically thought out usage.

Therefore I see no reason to not make these explicit aliases as it will only ease transition in the future.

ckissane added inline comments.Jul 1 2022, 11:25 AM
llvm/lib/ProfileData/InstrProf.cpp
155

And the other similar changes?
I will note these are NFC because if zlib is being used (as it currently always is), it has the exact same output.

MaskRay accepted this revision.Jul 1 2022, 11:44 AM

LGTM. Happy when @phosek is happy

ckissane updated this revision to Diff 441757.Jul 1 2022, 11:50 AM

discard debug string changes

ckissane updated this revision to Diff 441763.Jul 1 2022, 11:59 AM
ckissane edited the summary of this revision. (Show Details)

remove compression namespace alias work

ckissane marked 2 inline comments as done.Jul 1 2022, 12:01 PM

comments addressed

phosek accepted this revision.Jul 1 2022, 12:04 PM

LGTM

MaskRay accepted this revision.Jul 7 2022, 10:35 AM
MaskRay added inline comments.
llvm/include/llvm/Support/Compression.h
27

Is it still used?

Prefer StringRef if the string is backed from some storage.

llvm/unittests/Support/CompressionTest.cpp
21

Delete blank line between two using

ckissane added inline comments.Jul 7 2022, 11:27 AM
llvm/include/llvm/Support/Compression.h
27

No it is not, I will remove it and reintroduce it in the first patch that adds zstd

llvm/unittests/Support/CompressionTest.cpp
21

will do

I think this can be pushed now. You need to remove the variable

In file included from /var/lib/buildkite-agent/builds/llvm-project/llvm/lib/MC/ELFObjectWriter.cpp:41:
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/Compression.h:27:30: error: constexpr variable cannot have non-literal type 'const std::string' (aka 'const basic_string<char>')
static constexpr std::string AlgorithmName = "zlib";

You likely need to run ninja check-llvm check-clang check-clang-tools check-lld check-lldb to ensure all relevant targets still build.
(You can use check-all if your lists of LLVM_ENABLED_PROJECTS/LLVM_ENABLE_RUNTIMES are appropriate.)

llvm/include/llvm/Support/Compression.h
48

No need to add blank line between two namespace }.

llvm/lib/Support/Compression.cpp
25

delete the excess blank line

ckissane updated this revision to Diff 443005.Jul 7 2022, 11:40 AM
  • Merge branch 'ckissane.refactor-compression.part-0' of github.com:ckissane/llvm-project into ckissane.refactor-compression.part-0
  • Merge branch 'main' into ckissane.refactor-compression.part-0
  • compression refactor: undo string subs
  • Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-0.no-string-sub
  • remove compression namespaace aliases
  • update release notes to reflect no compression aliases
  • chore: fmt and remove AlgorithmName from zlib namespace
  • Merge remote-tracking branch 'origin/main' into ckissane.refactor-compression.part-0.no-string-sub
ckissane updated this revision to Diff 443006.Jul 7 2022, 11:41 AM
  • chore: delete an excess blank line
ckissane marked 5 inline comments as done.Jul 7 2022, 11:42 AM

mark some handled comments done

MaskRay accepted this revision.Jul 7 2022, 12:36 PM
This revision was landed with ongoing or failed builds.Jul 8 2022, 11:19 AM
This revision was automatically updated to reflect the committed changes.