modules/zstd: Add ZstdDecoder top level proc #1315

lpawelcz · 2024-02-21T16:13:42Z

This PR supersedes #1169
It is a part of #1211.
NOTE: this is based on #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.

proppy · 2024-02-28T20:59:59Z

@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 · 2024-02-28T21:01:38Z

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

proppy · 2024-02-28T21:05:35Z

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 · 2024-03-06T15:26:40Z

xls/modules/zstd/zstd_dec_test.cc

+  this->ParseAndCompareWithZstd(frame.value());
+}
+
+//class ZstdDecoderSeededTest : public ZstdDecoderTest,


any reason this is commented out?

A single test case from this test suite generate a random ZSTD frame with decodecorpus utility. It is decoded with libzstd and then the same encoded frame is processed through the simulation of ZSTD Decoder. The output of the simulation is gathered and compared against results from libzstd decoding.
Such test case is repeated 50 times with generating ZSTD frames containing only RAW blocks and 50 times with generating only RLE blocks.
Currently, some of those test cases are failing and we are looking into those. For the time being we commented these tests out so that it would be easily visible if something unexpected will cause the CI to fail in this PR.

xls/modules/zstd/zstd_dec_test.cc

lpawelcz · 2024-03-07T11:00:12Z

@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 #1204? should we rebase?

Yes, the conflict was caused by changes introduced with regards to #1308, #1202 and #1204. Fixed with rebase

EDIT:
I've also temporarily included changes from #1326 as those are required for fixing physical design flows done with openroad.

proppy · 2024-03-14T06:03:34Z

xls/modules/zstd/BUILD

+    data = [
+        ":zstd_dec_verilog.ir",
+    ],
+    #shard_count = 50,


is that intentionally commented?

Yes, this is useful for running ZstdDecoderSeededTest which are now commented out as we work on fixing all cases tested there.

proppy · 2024-03-14T06:04:55Z

xls/modules/zstd/BUILD

+)
+
+cc_test(
+    name = "zstd_dec_cc_test",


this currently PASS 👍 but with the following warning:

WARNING: //xls/modules/zstd:zstd_dec_cc_test: Test execution time (19.1s excluding execution overhead) outside of range for MODERATE tests. Consider setting timeout="short" or size="small".

Test execution time will surely change after enabling back the ZstdDecoderSeededTest. Thanks for pointing that out. We will make sure to set those attributes correctly.

proppy · 2024-03-14T06:21:26Z

xls/modules/zstd/zstd_dec_test.cc

@@ -0,0 +1,241 @@
+// Copyright 2020 The XLS Authors


this seems to fail w/ a timeout when introducing a deliberate failure (i.e: changing MAGIC_NUMBER from u32:0xFD2FB528; to u32:0xFD2FB527;

Target //xls/modules/zstd:zstd_dec_cc_test up-to-date: bazel-bin/xls/modules/zstd/zstd_dec_cc_test INFO: Elapsed time: 300.804s, Critical Path: 300.53s INFO: 3 processes: 3 linux-sandbox. INFO: Build completed, 1 test FAILED, 3 total actions //xls/modules/zstd:zstd_dec_cc_test TIMEOUT in 300.0s /usr/local/google/home/proppy/.cache/bazel/_bazel_proppy/fb962cb496438c85ace9ac1a1a0be573/execroot/com_google_xls/bazel-out/k8-fastbuild/testlogs/xls/modules/zstd/zstd_dec_cc_test/test.log Executed 1 out of 1 test: 1 fails locally.

is that expected? or is there a way to make it fail earlier?
(attached the test.log)

We would imagine that in the case of passing invalid ZSTD frame (e.g. wrong MAGIC_NUMBER), decoder should spin in DECODE_MAGIC_NUMBER state trying to detect it in incoming data packets and discarding the oldest byte in the buffer each time it failed. In your case decoder got stuck in ERROR state which requires restarting the decoder and we will be fixing that.

When it comes to failing early we'll have to look closely if this is possible.

proppy · 2024-03-14T06:24:47Z

xls/modules/zstd/README.md

+# ZSTD decoder
+
+The ZSTD decoder decompresses the correctly formed ZSTD frames and blocks.
+It implements the [RFC 8878](https://www.rfc-editor.org/rfc/rfc8878.html) decompression algorithm.


maybe add paragraph breaks (extra newline) here and in other paragraphs below?

proppy · 2024-03-14T06:26:04Z

xls/modules/zstd/README.md

+
+### Top level Proc
+This state machine is responsible for receiving encoded ZSTD frames, buffering the input and passing it to decoder's internal components based on the state of the proc.
+The states defined for the processing of ZSTD frame are as follows:


state diagram with https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-diagrams#creating-mermaid-diagrams (instead of image files)?

proppy · 2024-03-14T06:26:27Z

xls/modules/zstd/README.md

+* FEED_BLOCK_DECODER
+* DECODE_CHECKSUM
+
+After going through initial stages of decoding magic number and frame header, decoder starts the block division process.


what about inlining the description under of each state bullet point?

xls/modules/zstd/README.md

proppy · 2024-03-14T06:29:42Z

xls/modules/zstd/README.md

+After transmitting all data required for current block, it loops around to the block header decoding state and when next block header is not found it decodes checksum when it was requested in frame header or finishes ZSTD frame decoding and loops around to magic number decoding.
+
+### ZSTD frame header decoder
+This part of the design starts with detecting the ZSTD magic number.


s/This part of the design/This module/ ? to match with other description?

Frame header decoding is actually implemented as a series of functions that are called in the top level proc. I'm not sure if module would be a good word here.

xls/modules/zstd/README.md

proppy · 2024-03-14T06:33:34Z

xls/modules/zstd/README.md

+* FEED_BLOCK_DECODER
+* DECODE_CHECKSUM
+
+After going through initial stages of decoding magic number and frame header, decoder starts the block division process.


it feels like we're also somehow repeating what we say just below?

in the Top level Proc paragraph we provide only a short description of each stage so that it would be easier to get gist of the flow. After that, each particular stage is described in detail.

xls/modules/zstd/README.md

proppy · 2024-03-14T06:39:56Z

xls/modules/zstd/README.md

+
+## Known Limitations
+
+* **[WIP]** Uses old version of `SequenceExecutor` (up-to-date version is available in [google/xls/pull/1295](https://github.com/google/xls/pull/1295)) due to [reported issues](https://github.com/google/xls/pull/1295#issuecomment-1943857515) with verilog generation


@hongted is that a workaround we can share externally for #1018?

proppy · 2024-03-14T06:43:01Z

xls/modules/zstd/README.md

+
+![](img/ZSTD_decoder.png)
+
+## ZSTD decoder architecture


can we add link/pointed to the corresponding source(s) for each section?

This commit marks SimultaneousReadWriteBehavior enum and num_partitions function as public to allow for creating simpler tests that interact with RAM models. Internal-tag: [#53241] Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Internal-tag: [#54705] Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

This commit adds RAM printer block usefull for debugging HistoryBuffer inside SequenceExecutor. Internal-tag: [#54705] Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Add Proc responsible for handling ZSTD Sequence Execution step, which is described in: https://datatracker.ietf.org/doc/html/rfc8878#name-sequence-execution Internal-tag: [#54705] Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Internal-tag: [#52954] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

This commit adds a ZSTD decoder module that parses ZSTD frames. The provided tests examine the model using C++ API, which is a prerequisite for detailed tests using zstd library. Internal-tag: [#50221] Co-authored-by: Maciej Dudek <mdudek@antmicro.com> Co-authored-by: Pawel Czarnecki <pczarnecki@antmicro.com> Signed-off-by: Maciej Dudek <mdudek@antmicro.com> Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Required for spawning SequenceExecutor in ZSTD top level proc. Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

…ons public Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

…e==0 Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Internal-tag: [#52186] Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

proppy · 2024-05-17T13:50:22Z

@hongted and I where discussing if we should split the zstd CI from the main workflow w/ additional pre-conditions:

only run test //xls/modules/zstd/... when an incoming PR affected xls/modules/**
still run build //xls/modules/zstd/...` in the main CI

That way we can check-in if a given toolchain CL break the zstd modules (i.e: syntax change), but we can still check-in the zstd module w/ failing tests (so that we can more easily evaluate the PR w/ missing feature).

What do you think?

rw1nkler · 2024-05-23T20:58:32Z

@hongted and I where discussing if we should split the zstd CI from the main workflow w/ additional pre-conditions:
* only run `test //xls/modules/zstd/...` when an incoming PR affected `xls/modules/**`

* still run `build `//xls/modules/zstd/...` in the main CI
That way we can check-in if a given toolchain CL break the zstd modules (i.e: syntax change), but we can still check-in the zstd module w/ failing tests (so that we can more easily evaluate the PR w/ missing feature).

What do you think?

We use many place_and_route build rules that are rather long. Should we include them in the main CI, or filter them out and skip?

proppy · 2024-05-24T06:03:02Z

We use many place_and_route build rules that are rather long. Should we include them in the main CI, or filter them out and skip?

I think those should be part of the zstd specific CI.

lpawelcz mentioned this pull request Feb 22, 2024

modules/zstd: add buffer module #1167

Closed

lpawelcz force-pushed the 52186-zstd-top branch from 71be328 to 84b31f3 Compare February 22, 2024 10:33

proppy self-requested a review February 22, 2024 17:52

proppy reviewed Mar 6, 2024

View reviewed changes

xls/modules/zstd/zstd_dec_test.cc Show resolved Hide resolved

lpawelcz force-pushed the 52186-zstd-top branch from 84b31f3 to 8f41b67 Compare March 7, 2024 10:52