Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613

rw1nkler · 2024-09-18T15:05:11Z

This PR introduces pros that enable the interface between DSLX modules and AXI subordinates. The attached README provides detailed documentation of the new functionality. Provided Verilog simulations show that the new procs allow for reading from and writing to RAM located on the AXI bus.

While working on this PR, we encountered a potential issue linked to afc720d, which appears to cause indefinite parsing and type-checking loops for certain files, such as mem_writer.x (provided in this PR).

google-cla · 2024-09-18T15:05:17Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

lpawelcz · 2024-09-20T15:24:18Z

Moved the cocotb tests to a separate PR: #1616

lpawelcz · 2024-09-24T09:16:40Z

Added performance improvements for the MemWriter proc.

The PR is ready for review, @proppy please take a look.

xls/modules/zstd/memory/BUILD

proppy · 2024-09-24T13:46:19Z

xls/modules/zstd/memory/BUILD

+    dslx_top = "AxiReaderInst",
+    library = ":axi_reader_dslx",
+    opt_ir_args = {
+        "top": "__axi_reader__AxiReaderInst__AxiReader_0__16_32_4_4_4_3_2_14_next",


is that still required?

Yes, it is required for IR benchmarks because XLS does not allow optimizing IRs of procs that have empty next().

This is very often the case for parameterized procs.
We must specify additional Instance proc that spawns the proc in question with specified parameters.
In such case the proc marked as default top in the IR will be the Instance proc which has empty next(). If we want to benchmark the IR, it is required to point to the internal proc as the new top because this proc contains all of the logic.

proppy · 2024-09-24T13:47:23Z

xls/modules/zstd/memory/BUILD

+    "streaming_channel_data_suffix": "_data",
+    "flop_inputs_kind": "skid",
+    "flop_outputs_kind": "skid",
+    "clock_period_ps": "1800",


any reason the clock period is different here? should we keep a list of those at the top for easier tuning?

The critical path delay from the IR benchmark for this proc exceeds the 750ps that we use throughout the ZSTD Decoder. In order to provide valid constraints for the scheduler we increased the clock period to the value that is close to the critical path delay. Moved this to a variable at the top of the file where the 750ps clock period is also defined.

proppy · 2024-09-24T13:47:30Z

xls/modules/zstd/memory/BUILD

+
+axi_stream_remove_empty_codegen_args = common_codegen_args | {
+    "module_name": "axi_stream_remove_empty",
+    "clock_period_ps": "1300",


any reason the clock period is different here? should we keep a list of those at the top for easier tuning?

The critical path delay from the IR benchmark for this proc exceeds the 750ps that we use throughout the ZSTD Decoder. In order to provide valid constraints for the scheduler we increased the clock period to the value that is close to the critical path delay. Moved this to a variable at the top of the file where the 750ps clock period is also defined.

proppy · 2024-09-24T13:47:38Z

xls/modules/zstd/memory/BUILD

+    "streaming_channel_data_suffix": "_data",
+    "flop_inputs_kind": "skid",
+    "flop_outputs_kind": "skid",
+    "clock_period_ps": "2600",


any reason the clock period is different here? should we keep a list of those at the top for easier tuning?

The critical path delay from the IR benchmark for this proc exceeds the 750ps that we use throughout the ZSTD Decoder. In order to provide valid constraints for the scheduler we increased the clock period to the value that is close to the critical path delay. Moved this to a variable at the top of the file where the 750ps clock period is also defined.

proppy · 2024-09-24T14:13:01Z

xls/modules/zstd/memory/README.md

+4. Wait for the response submitted on the `resp_s` channel, which indicates
+   if the write operation was successful or an error occurred.
+
+# Cocotb Simulation


can we move this section to #1616?

proppy · 2024-09-24T14:24:01Z

The tests take quite a long time to execute:

[4,813 / 4,816] 18 / 21 tests; Parsing and type checking DSLX source files of target axi_writer_dslx; 2688s linux-sandbox ... (3 actions running)

is that expected?

rw1nkler · 2024-09-24T15:00:29Z

The tests take quite a long time to execute:

[4,813 / 4,816] 18 / 21 tests; Parsing and type checking DSLX source files of target axi_writer_dslx; 2688s linux-sandbox ... (3 actions running)

is that expected?

This is the problem described in #1615. Reverting afc720d, allowed us to pass the type-checking step, but most probably the issue requires a proper fix, rather than reverting the functionality.

proppy · 2024-10-01T23:41:01Z

according to #1615 (comment), does this need to be updated?

lpawelcz · 2024-10-04T13:03:58Z

@proppy
We rebased the PR and fixed the codebase according to #1615 (comment).

After the rebase we experienced a regression in place_and_route rule for the SequenceExecutor, hence the 9282bfb.

Additionally, we noticed that bazel run calls in https://github.com/google/xls/blob/main/.github/workflows/modules-zstd.yml#L63 execute only the first bazel target from the bazel query output.
In 94a6d6d we fixed this by explicitly looping through all the targets and calling bazel run for one target at a time.

We addressed your review comments and slightly changed the interface of the MemReader/Writer by removing the Ctrl channel that was used to configure the procs with a base address. Now the requests to the procs contain absolute addresses instead of offsets from the base address.

proppy · 2024-10-10T02:54:57Z

After the rebase we experienced a regression in place_and_route rule for the SequenceExecutor, hence the 9282bfb.

did you report it to https://github.com/hdl/bazel_rules_hdl ? /cc @mikesinouye @QuantamHD

proppy · 2024-10-10T02:57:28Z

Additionally, we noticed that bazel run calls in https://github.com/google/xls/blob/main/.github/workflows/modules-zstd.yml#L63 execute only the first bazel target from the bazel query output.

That sounds weird, can you share the shell-expanded bazel command line that only run the first target?

proppy · 2024-10-10T02:58:51Z

xls/modules/zstd/memory/BUILD

+)
+
+CLOCK_PERIOD_PS = "750"
+# Clock periods for modules that exceed the 750ps critical path in IR benchmark


would increasing pipelining help?

This is not enough for the scheduler as we constrain it from 3 sides:

worst case throughput - 1

target clock period - 750ps

pipeline stages - as many as required

Combining worst_case_throughput==1 with fixed clock period requirement enforces a condition that each next() evaluation must be possible to compute in a given clock period time limit.

Currently, some of the procs don't meet this requirement and scheduling in such case will fail with a message to increase the worst_case_throughput. We can do that or we can stop enforcing specific clock period for such procs. This way, the module would still evaluate the next() function in a single clock cycle and we would still have an estimate for the max clock period from the IR benchmark.

proppy · 2024-10-10T03:00:29Z

xls/modules/zstd/memory/BUILD

+    src = ":axi_reader_verilog.opt.ir",
+    benchmark_ir_args = axi_reader_codegen_args | {
+        "pipeline_stages": "10",
+        "top": "__axi_reader__AxiReaderInst__AxiReader_0__16_32_4_4_4_3_2_14_next",


maybe use https://google.github.io/xls/bazel_rules_macros/#get_mangled_ir_symbol?

It seems that this rule works mostly for the simple DSLX functions. I was not able generate a correct name of the proc with it. Tried e.g.:

get_mangled_ir_symbol("AxiReaderInst", "AxiReader", (16, 32, 4, 4, 4, 3, 2, 14)), got: __AxiReaderInst__AxiReader__16_32_4_4_4_3_2_14

get_mangled_ir_symbol("AxiReaderInst", "AxiReader", is_proc_next=True) , got: __AxiReaderInst__AxiReadernext

Looks like the case of a next() function of an instance of the parameterized proc that is spawned in other proc is not supported.

ah you're right:

is_proc_next: A boolean flag denoting whether the symbol is a next proc function. The argument is mutually exclusive with arguments: 'parametric_values' and 'is_implicit_token'.

We should file a feature request for this to be supported, one workaround I can see is to define wrapper proc rather than putting the parameters in the BUILD file.

lpawelcz · 2024-10-10T06:03:43Z

Additionally, we noticed that bazel run calls in https://github.com/google/xls/blob/main/.github/workflows/modules-zstd.yml#L63 execute only the first bazel target from the bazel query output.

That sounds weird, can you share the shell-expanded bazel command line that only run the first target?

That would be for example:

bazel run -c opt -- //xls/modules/zstd:axi_csr_accessor_opt_ir_benchmark //xls/modules/zstd:block_header_dec_opt_ir_benchmark //xls/modules/zstd:csr_config_opt_ir_benchmark //xls/modules/zstd:dec_mux_opt_ir_benchmark //xls/modules/zstd:frame_header_dec_opt_ir_benchmark //xls/modules/zstd:raw_block_dec_opt_ir_benchmark //xls/modules/zstd:repacketizer_opt_ir_benchmark //xls/modules/zstd:rle_block_dec_opt_ir_benchmark //xls/modules/zstd:sequence_executor_opt_ir_benchmark //xls/modules/zstd:window_buffer_opt_ir_benchmark //xls/modules/zstd:zstd_dec_internal_opt_ir_benchmark //xls/modules/zstd/memory:axi_reader_opt_ir_benchmark //xls/modules/zstd/memory:axi_stream_add_empty_opt_ir_benchmark //xls/modules/zstd/memory:axi_stream_downscaler_opt_ir_benchmark //xls/modules/zstd/memory:axi_stream_remove_empty_opt_ir_benchmark //xls/modules/zstd/memory:axi_writer_opt_ir_benchmark //xls/modules/zstd/memory:mem_reader_internal_opt_ir_benchmark

This will execute the IR benchmark only for the axi_csr_accessor proc.

bazel run executes only the first target from the list of targets acquired from the output of bazel query. In order to properly call all targets it is required to loop through the targets and run one at a time Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Required to pass place_and_route Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

Co-authored-by: Michal Czyz <mczyz@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

This comit adds implementation of AxiReader proc that can be used to to issue AXI read requests as an AXI Manager device. Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

This commits adds AxiStreamRemoveEmpty proc, that can be used to remove bytes marked as containing no data in the Axi Stream Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

This commit adds AxiStreamDownscaler that can be used to convert AxiStream transactions from a wider bus, to multiple transactions on more narrow bus Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

This commit adds MemReader and MemReaderAdv procs for handling read transactions on the AXI bus. Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

Internal-tag: [#62924] Co-authred-by: Pawel Czarnecki <pczarnecki@antmicro.com> Co-authred-by: Robert Winkler <rwinkler@antmicro.com> Signed-off-by: Michal Czyz <mczyz@antmicro.com> Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

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

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

Co-authored-by: Pawel Czarnecki <pczarnecki@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

proppy · 2024-10-17T05:39:39Z

This will execute the IR benchmark only for the axi_csr_accessor proc.

Looks like this is by design:
https://bazel.build/docs/user-manual#running-executables

The bazel run command is similar to bazel build, except it is used to build and run a single target.

hzeller · 2024-10-17T17:27:50Z

xls/modules/zstd/memory/BUILD

+load("@rules_hdl//place_and_route:build_defs.bzl", "place_and_route")
+load("@rules_hdl//synthesis:build_defs.bzl", "benchmark_synth", "synthesize_rtl")
+load("@rules_hdl//verilog:providers.bzl", "verilog_library")
+load("@xls_pip_deps//:requirements.bzl", "requirement")


This looks like an unused import.

Can you run

buildifier --lint=fix xls/modules/zstd/memory/BUILD

on it ? (Sorry, we should have built into the CI)

Fixing this now on import, but it is always a good idea to run buildifier before.

rw1nkler force-pushed the axi branch from 72e55dd to 4018d32 Compare September 18, 2024 19:04

lpawelcz mentioned this pull request Sep 19, 2024

Indefinite parsing and type-checking loops for some parameterized procs #1615

Closed

lpawelcz force-pushed the axi branch 3 times, most recently from 4ca2f68 to 013deef Compare September 20, 2024 15:23

lpawelcz mentioned this pull request Sep 20, 2024

Add cocotb tests for AXI peripherals #1616

Open

lpawelcz force-pushed the axi branch from 013deef to bd328e2 Compare September 23, 2024 10:51

proppy reviewed Sep 24, 2024

View reviewed changes

xls/modules/zstd/memory/BUILD Show resolved Hide resolved

proppy reviewed Sep 24, 2024

View reviewed changes

lpawelcz force-pushed the axi branch from bd328e2 to 8e60d89 Compare October 4, 2024 13:02

lpawelcz force-pushed the axi branch from 8e60d89 to 2275776 Compare October 9, 2024 11:57

lpawelcz mentioned this pull request Oct 9, 2024

modules/zstd: Rework ZSTD Decoder #1654

Draft

7 tasks

proppy reviewed Oct 10, 2024

View reviewed changes

lpawelcz added 2 commits October 11, 2024 10:09

modules/zstd: Decrease pin spacing for SequenceExecutor

2d253f2

Required to pass place_and_route Signed-off-by: Pawel Czarnecki <pczarnecki@antmicro.com>

rw1nkler and others added 10 commits October 11, 2024 10:09

modules/zstd/memory: Add packages with AXI4 structs

6d904a4

Co-authored-by: Michal Czyz <mczyz@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add common definitions for memory blocks

7c18c98

Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add AxiReader proc

788dc78

This comit adds implementation of AxiReader proc that can be used to to issue AXI read requests as an AXI Manager device. Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add AxiStreamRemoveEmpty proc

fb11f56

This commits adds AxiStreamRemoveEmpty proc, that can be used to remove bytes marked as containing no data in the Axi Stream Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add AxiStreamDownscaler proc

8dd3b30

This commit adds AxiStreamDownscaler that can be used to convert AxiStream transactions from a wider bus, to multiple transactions on more narrow bus Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add MemReader and MemReaderAdv

f34d8f1

This commit adds MemReader and MemReaderAdv procs for handling read transactions on the AXI bus. Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

modules/zstd/memory: Add AxiStreamAddEmpty

31d9bd6

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

modules/zstd/memory: Add MemWriter

7eb6798

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

modules/zstd/memory: Add README describing the AXI interface

c8ee6a9

Co-authored-by: Pawel Czarnecki <pczarnecki@antmicro.com> Signed-off-by: Robert Winkler <rwinkler@antmicro.com>

lpawelcz force-pushed the axi branch from 2275776 to c8ee6a9 Compare October 11, 2024 12:43

proppy approved these changes Oct 17, 2024

View reviewed changes

proppy added the Reviewing Internally label Oct 17, 2024

hzeller reviewed Oct 17, 2024

View reviewed changes

copybara-service bot merged commit 1130740 into google:main Oct 21, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613

Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613

rw1nkler commented Sep 18, 2024 •

edited

Loading

google-cla bot commented Sep 18, 2024

lpawelcz commented Sep 20, 2024

lpawelcz commented Sep 24, 2024

proppy Sep 24, 2024

lpawelcz Oct 4, 2024

proppy Sep 24, 2024

lpawelcz Oct 4, 2024

proppy Sep 24, 2024

lpawelcz Oct 4, 2024

proppy Sep 24, 2024

lpawelcz Oct 4, 2024

proppy Sep 24, 2024

lpawelcz Oct 4, 2024

proppy commented Sep 24, 2024

rw1nkler commented Sep 24, 2024 •

edited

Loading

proppy commented Oct 1, 2024

lpawelcz commented Oct 4, 2024 •

edited

Loading

proppy commented Oct 10, 2024 •

edited

Loading

proppy commented Oct 10, 2024

proppy Oct 10, 2024

lpawelcz Oct 11, 2024

proppy Oct 10, 2024

lpawelcz Oct 11, 2024

proppy Oct 17, 2024

lpawelcz commented Oct 10, 2024

proppy commented Oct 17, 2024

hzeller Oct 17, 2024

hzeller Oct 21, 2024

Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613

Add procs for interfacing with AXI peripherals in ZSTD Decoder #1613

Conversation

rw1nkler commented Sep 18, 2024 • edited Loading

google-cla bot commented Sep 18, 2024

lpawelcz commented Sep 20, 2024

lpawelcz commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy commented Sep 24, 2024

rw1nkler commented Sep 24, 2024 • edited Loading

proppy commented Oct 1, 2024

lpawelcz commented Oct 4, 2024 • edited Loading

proppy commented Oct 10, 2024 • edited Loading

proppy commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpawelcz commented Oct 10, 2024

proppy commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw1nkler commented Sep 18, 2024 •

edited

Loading

rw1nkler commented Sep 24, 2024 •

edited

Loading

lpawelcz commented Oct 4, 2024 •

edited

Loading

proppy commented Oct 10, 2024 •

edited

Loading