xls
xls copied to clipboard
modules/zstd: Add ZstdDecoder top level proc
This PR supersedes https://github.com/google/xls/pull/1169
It is a part of https://github.com/google/xls/issues/1211.
NOTE: this is based on https://github.com/google/xls/pull/1314.
Please ignore all commits with [TEMP]
in commit message when reviewing. Those are squashed commits of all previous PRs (links are available in the second line of each commit message)
This PR adds a top level proc for ZSTD Decoder which integrates all its components in order to allow decoding of ZSTD frames. It includes C++ tests which:
- generate with
decodecorpus
tool valid ZSTD frames - decode the frames with
libzstd
andZstdDecoder
- compare the results
NOTE: currently it is possible to decode frames with simple block types as RAW and RLE. There are however still some issues with decoder implementation as not all tests are passing, hence the commented out tests with generating random frames.
@lpawelcz can you add some markdown documentation on how to test the implementation against decorecorpus
? (even if that just reference the blaze rules to run)
is the conflict in ir_convert
because of #1204? should we rebase?
NOTE: currently it is possible to decode frames with simple block types as RAW and RLE. There are however still some issues with decoder implementation as not all tests are passing, hence the commented out tests with generating random frames.
Would it make sense to add a README.md w/ a known limitation section in the zstd directory to document this?
@lpawelcz can you add some markdown documentation on how to test the implementation against
decorecorpus
? (even if that just reference the blaze rules to run)
Would it make sense to add a README.md w/ a known limitation section in the zstd directory to document this?
We added a README.md
which describes the ZSTD Decoder, its components, how to test against libzstd
and what are the known limitations of the decoder in the current state.
is the conflict in ir_convert because of https://github.com/google/xls/pull/1204? should we rebase?
Yes, the conflict was caused by changes introduced with regards to https://github.com/google/xls/issues/1308, https://github.com/google/xls/issues/1202 and https://github.com/google/xls/pull/1204. Fixed with rebase
EDIT:
I've also temporarily included changes from https://github.com/google/xls/pull/1326 as those are required for fixing physical design flows done with openroad
.
can you remove the Draft
label since this is ready for review?
CI has been going for 5+ hours, https://github.com/google/xls/actions/runs/8541194594/job/23399959193
can we find a way to exclude the zstd test from the main CI? (we could have a separate daily workflow that run them)
Sure, we are working on a CI workflow similar to the one in https://github.com/google/xls/pull/1215