xls icon indicating copy to clipboard operation
xls copied to clipboard

modules/zstd: Add ZstdDecoder top level proc

Open lpawelcz opened this issue 1 year ago • 30 comments

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 and ZstdDecoder
  • 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 avatar Feb 21 '24 16:02 lpawelcz

@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)

proppy avatar Feb 28 '24 20:02 proppy

is the conflict in ir_convert because of #1204? should we rebase?

proppy avatar Feb 28 '24 21:02 proppy

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?

proppy avatar Feb 28 '24 21:02 proppy

@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.

lpawelcz avatar Mar 07 '24 11:03 lpawelcz

can you remove the Draft label since this is ready for review?

proppy avatar Apr 03 '24 08:04 proppy

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)

proppy avatar Apr 03 '24 21:04 proppy

Sure, we are working on a CI workflow similar to the one in https://github.com/google/xls/pull/1215

lpawelcz avatar Apr 04 '24 06:04 lpawelcz