Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory-efficient zlib usage across Liberty file consumers #4834

Merged
merged 11 commits into from
Mar 20, 2025

Conversation

widlarizer
Copy link
Collaborator

In ASIC flows, Liberty files provided in the PDK can be very large and are often compressed with gzip. Prior to this PR, the only pass that accepted .lib.gz was read_liberty, just like read_verilog accepts .v.gz etc. -liberty filename arguments to passes like clockgate and dfflibmap didn't use the decompression code.

The decompress_gzip function also has been reading the entirety of the uncompressed file into RAM (into a stringstream). This is now fixed by implementing an gzip_istream much like the pre-existing gzip_ostream. It has one byte of guaranteed unget() which is sufficient for our Liberty parsing. Using gzstream was ruled out because it's LGPL.

The PR carries also some shuffling of things around which should have limited impact on plugin devs since the new kernel/io.h header is included in kernel/yosys_common.h.

This will resolve #4830 but only on the Yosys side. The Yosys abc pass only hands off the paths specified with abc -liberty path to abc but abc doesn't support .lib.gz which is surprising since it does have its own vendored zlib and wrapper around it used for aiger

Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only reviewed gzip.cc/gzip.h, it looks fine except for the nits

int bytes_read = Zlib::gzread(gzf, buffer, buffer_size);
if (bytes_read <= 0) {
if (Zlib::gzeof(gzf))
return traits_type::eof();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading https://en.cppreference.com/w/cpp/io/basic_streambuf/underflow we might need to do something about "On failure, the function ensures that either gptr() == nullptr or gptr() == egptr." because the invariant of when this function is called is weaker, it is "The public functions of std::streambuf call this function only if gptr() == nullptr or gptr() >= egptr()."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, now I setg(eback(), egptr(), egptr()); before returning eof

@widlarizer
Copy link
Collaborator Author

widlarizer commented Jan 22, 2025

  • Look at stat -liberty

@KrystalDelusion
Copy link
Member

Is there a reason io.cc doesn't use the <filesystem> standard library? It already provides creating/deleting files/directories, temp directory provision, file existence checking.

@widlarizer
Copy link
Collaborator Author

@KrystalDelusion since that's code I really only moved around without really changing, I think that's a followup issue - if I was more aware of it I may have refactored it to use <filesystem> but I don't have the capacity to do so immediately and would like to merge this

@whitequark wasn't there a WASI blocker in relation to <filesystem> and threads? I recall a discussion about that but can't find it on github

@whitequark
Copy link
Member

@whitequark wasn't there a WASI blocker in relation to <filesystem> and threads? I recall a discussion about that but can't find it on github

There was yeah. I think <filesystem> doesn't work without threads and thread functions aren't available in single-threaded WASI model until a release of wasi-libc that includes WebAssembly/wasi-libc#518.

I would say this is probably no longer a blocker but someone™ needs to build and test the Wasm port.

@KrystalDelusion
Copy link
Member

someone™ needs to build and test the Wasm port

I take it that's more than just the WASI test build included in CI?

@KrystalDelusion
Copy link
Member

KrystalDelusion commented Mar 19, 2025

.../yosys-src/tests/various$ yowasp-yosys stat.ys

 /----------------------------------------------------------------------------\
 |  yosys -- Yosys Open SYnthesis Suite                                       |
 |  Copyright (C) 2012 - 2025  Claire Xenia Wolf <[email protected]>         |
 |  Distributed under an ISC-like license, type "license" to see terms        |
 \----------------------------------------------------------------------------/
 Yosys 0.51+41 (git sha1 980a0a15c, ccache clang 18.1.2 -O3 -flto -flto)

-- Executing script file `stat.ys' --

1. Executing RTLIL frontend.
Input filename: <<EOF
Added regex 'Chip area for module '\\top': 9.072000' to expected log messages list.
ERROR: File `../../tests/liberty/foundry_data/sg13g2_stdcell_typ_1p20V_25C.lib.filtered.gz' is a gzip file, but Yosys is compiled without zlib.
ERROR: Expected log pattern 'Chip area for module '\\top': 9.072000' not found !

Hm. So it doesn't work in yowasp-yosys, but it also hasn't broken anything. yowasp-yosys tests/techmap/shiftx2mux.ys passes (along with a few other random tests I tried), which should be exercising the temp file stuff which moved. I can also run a synth_ice40 with json output, and show output to file. This should be fine to merge imo.

@whitequark
Copy link
Member

Sounds good to me, thanks for testing @KrystalDelusion!

@KrystalDelusion KrystalDelusion added the merge-soon Merge: PR will be merged at the end of the next work day unless concerns are raised label Mar 20, 2025
@KrystalDelusion KrystalDelusion merged commit b06a661 into main Mar 20, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-soon Merge: PR will be merged at the end of the next work day unless concerns are raised
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support gziped Liberty files
4 participants