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

modules/zstd: add Zstd decoder skeleton #1169

Closed
wants to merge 12 commits into from

Conversation

rdob-ant
Copy link
Contributor

This PR adds a skeleton of Zstd decoder that for now only parses a frame header (using DSLX modules added by other Zstd PRs). The module itself is in DSLX, while tests are in C++ as they need to use file IO and will also call into libzstd in the future. As this is a skeleton only, it'll most probably undergo substantial changes when other Zstd parser modules are added.

NOTE: this is based on #1168 , please ignore commits from that branch when reviewing.

rw1nkler and others added 12 commits October 25, 2023 11:55
This commit adds rules to build zstd library and exposes the following targets:
* `@com_github_facebok_zstd//:libzstd` - zstd library itself
* `@com_github_facebok_zstd//:zstd` - binary for performing zstd compression/decompression
* `@com_github_facebok_zstd//:decodecorpus` - binary for creating test data for a zstd decoder

Internal-tag: #[49966]
Signed-off-by: Robert Winkler <[email protected]>
This commit enables building zstd library in version 1.5.5 in
the XLS repository. The sources of the library come from
the official release of zstd on Meta's GitHub.

Internal-tag: #[49966]
Signed-off-by: Robert Winkler <[email protected]>
Disable building llvm with zstd to avoid conflicts with newly-added zstd
library. It seems that llvm_zstd is not necessary for correct llvm
operation.

Internal-tag: #[49966]
Signed-off-by: Robert Winkler <[email protected]>
This commit adds a DSLX Buffer library that provides the Buffer struct,
and helper functions that can be used to operate on it. The Buffer
is meant to be a storage for data coming from the channel. It acts like
a FIFO, allowing data of any length to be put in or popped out of it.
Provided DSLX tests verify the correct behaviour of the library.

Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
This commit adds the library with functions for parsing a magic number and
tests that verify its correctness.

Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
This commit adds the library with functions for parsing a frame header.
The provided tests verify the correcness of the library.

Internal-tag: [#49967]
Signed-off-by: Robert Winkler <[email protected]>
Internal-tag: [#49967]
Signed-off-by: Robert Winkler <[email protected]>
Fix module behavior when single_segment is true and FCS size
is set as 0.

Internal-tag: [#49967]
Signed-off-by: Roman Dobrodii <[email protected]>
This commit adds a skeleton of ZSTD decoder that parses a magic number and
a frame header. The provided tests examine the model using C++ API,
which is a prerequisite for detailed tests using zstd library.

Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
Internal-tag: [#50221]
Signed-off-by: Robert Winkler <[email protected]>
@proppy
Copy link
Member

proppy commented Oct 26, 2023

while tests are in C++ as they need to use file IO and will also call into libzstd in the future

👍 for writing test in C++ that compare against libzstd.

DECODE_BLOCKS = 2,
}

struct ZstdDecoderState {
Copy link
Member

@proppy proppy Oct 26, 2023

Choose a reason for hiding this comment

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

can we add inline doc string for each state members?

_ => state
};

let state = match state.status {
Copy link
Member

Choose a reason for hiding this comment

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

are you keeping the match separte so that you can process the frame header in the same tick? (if yes, is this state even necessary?)

Copy link
Contributor

Choose a reason for hiding this comment

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

With the current shape of the skeleton, the state is not necessary. However, soon we will extend it with the decoding capabilities that would make use of the internal state

const char *send_channel_name = "zstd_dec__input_r";

const char *ir_file = "xls/modules/zstd/zstd_dec_opt_ir.opt.ir";
const char *zstd_file = "xls/modules/zstd/data.zst";
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could generate this from the text fixture?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! We will add the ability to generate test data

@proppy proppy changed the title modules/zstd: add Zstd decoder skeleton (part 4) modules/zstd: add Zstd decoder skeleton Nov 9, 2023
@cdleary cdleary added the app Application level functionality (examples, uses of XLS stack) label Mar 27, 2024
@proppy
Copy link
Member

proppy commented Mar 29, 2024

should we close this and focus on reviewing #1315 ?

@tmichalak
Copy link

The review will happen in #1315, so this one can be closed.

@proppy proppy closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Application level functionality (examples, uses of XLS stack)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants