8000 Refine ZSTD decoder by rw1nkler · Pull Request #2538 · google/xls · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refine ZSTD decoder #2538

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Refine ZSTD decoder #2538

wants to merge 12 commits into from
+1,334 −578

Conversation

rw1nkler
Copy link
Contributor
@rw1nkler rw1nkler commented Jul 4, 2025

This PR introduces changes to the Sequence decoding logic in the ZSTD decoder.
The updates fix design behavior in Verilog simulation without breaking existing DSLX tests.
Most changes address issues that are not visible in DSLX simulation due to limitations of the DSLX interpreter and it's sequential nature. These include:

  • Fixing the ordering of operations by adjusting token passing
  • Fixing RAM rewriting in RamDemux, where artificial responses were send over muxed chanel even with no prior requests
  • Fixing an issue where FseLookupCtrl does not wait on recv_if before proceeding to the subsequent next() activation in Verilog simulation

We will investigate these problems further and open related issues if necessary.
Additionally, RAM sizes for FSE lookups were increased to match the values used in the ZSTD reference library.

FYI @proppy

proppy
proppy reviewed Jul 7, 2025
View reviewed changes
xls/modules/zstd/BUILD
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you spell constant in UPPERCASE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain what exactly do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

see https://bazel.build/build/style-guide#other-conventions

Copy link
Contributor Author
@rw1nkler rw1nkler Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would you like all those dicts defined between the rules to be in uppercase? Do I understand you correctly? If so - sure we can change that. Just let me know if this is what you expect.

Copy link

Choose a reason for hiding this comment

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

example: https://github.com/google/xls/pull/2538/files#diff-6ccd896d195812d5667d4fec944f7d703b12be91df036e9af9b74c50163904eaR1341 - let us know if that is what you mean

@proppy
Copy link
Member
proppy commented Jul 7, 2025

@rw1nkler is that ready to import/review?

@proppy
Copy link
Member
proppy commented Jul 7, 2025

The updates fix design behavior in Verilog simulation without breaking existing DSLX tests.

Are those part of the cocotb sim that are not imported yet?

@proppy
Copy link
Member
proppy commented Jul 7, 2025

Looks like one of the benchmark test is failing:

INFO: Running command line: bazel-bin/xls/modules/zstd/ram_demux_naive_opt_ir_benchmark.sh --logtostderr
Error: NOT_FOUND: Could not find top for this package; tried: ["__ram_demux__RamDemuxNaiveInst__RamDemuxNaive_0__5_8_0_8_next"]; available: "__ram_demux__RamDemuxNaiveFixedInst_0_next", "__ram_demux__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamDemuxNaive_0__10_64_0_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_0__RamPassthroughRead_0__10_64_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_0__RamPassthroughWrite_0__10_64_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_1__RamPassthroughRead_0__10_64_64_next", "__xls_modules_
8000
zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_1__RamPassthroughWrite_0__10_64_64_next"

rw1nkler and others added 10 commits July 7, 2025 12:41
@rw1nkler
modules: zstd: Add ram passthrough proc and wrappers for muxes/demuxes
fddb5c9 
This commit adds a passthrough proc which is attached to the output of
the RamDemux proc. Because of this, RAM rewriting is used on the outputs
of the passthrough instead of the outputs of the RamDemux.

This resolves the problem spotted in the Verilog simulation,
where artificial responses are sent from RamDemux despite
no request being made previously.

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>
@laudominik @rw1nkler
modules: zstd: extract FseLookupCtrl to internal proc
e701ecb
@laudominik @rw1nkler
modules: zstd: fix fse table ram sizes/address widths
d2a893e
@rw1nkler
modules: zstd: Use different max accuracy_log for tests
9f1cb18 
The smaller accuracy log results in smaller FSE decoding lookups, which
are needed to limit the RAM consumption in the DSLX interpreter.

This is related to the problems described in:
* google#1042
* google#1897

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>
@laudominik @rw1nkler
modules: zstd: persist accuracy logs for usage in repeated block deco…
7eec5e2 
…ding
@rw1nkler
modules: zstd: Simplify token ordering in RamDemux
d1466a0 
Signed-off-by: Robert Winkler <rwinkler@antmicro.com>
@laudominik @rw1nkler
modules: zstd: fix tokens in literals block header decoder
49312f4
@rw1nkler
modules: zstd: Fix condition for sending select response in Refilling…
6f11314 
…ShiftBufferMux

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>
@rw1nkler
modules: zstd: Use separate RAM interfaces instead of an array of cha…
b4530fb 
…nnels

Using separate RAM interfaces preserves the original port names in Verilog,
making it easier to understand the design's behavior during simulation.

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>
@laudominik @rw1nkler
modules: zstd: compress block decoder tests fixes
a5e063f
@rw1nkler rw1nkler force-pushed the github-pr-decoder-dslx-fixes branch from d5bdf45 to a5e063f Compare July 7, 2025 10:42
@rw1nkler
Copy link
Contributor Author
rw1nkler commented Jul 7, 2025

The updates fix design behavior in Verilog simulation without breaking existing DSLX tests.

Are those part of the cocotb sim that are not imported yet?

Yes, that is right

@rw1nkler
Copy link
Contributor Author
rw1nkler commented Jul 7, 2025

Looks like one of the benchmark test is failing:

INFO: Running command line: bazel-bin/xls/modules/zstd/ram_demux_naive_opt_ir_benchmark.sh --logtostderr
Error: NOT_FOUND: Could not find top for this package; tried: ["__ram_demux__RamDemuxNaiveInst__RamDemuxNaive_0__5_8_0_8_next"]; available: "__ram_demux__RamDemuxNaiveFixedInst_0_next", "__ram_demux__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamDemuxNaive_0__10_64_0_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_0__RamPassthroughRead_0__10_64_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_0__RamPassthroughWrite_0__10_64_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_1__RamPassthroughRead_0__10_64_64_next", "__xls_modules_zstd_ram_passthrough__RamDemuxNaiveFixedInst__RamDemuxNaiveFixed_0__RamPassthrough_1__RamPassthroughWrite_0__10_64_64_next"

This should be fixed now

@rw1nkler rw1nkler marked this pull request as ready for review July 7, 2025 10:43
@rw1nkler
Copy link
Contributor Author
rw1nkler commented Jul 7, 2025

@rw1nkler is that ready to import/review?

@proppy Yes, now it is ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@proppy proppy proppy left review comments

@mgielda mgielda mgielda left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rw1nkler @proppy @mgielda @laudominik
0