Remove unhelpful axi wrapper runt tests and cocotb tests #2515
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @nathanielnrn! I don't have time to review Calyx PRs right now so I will take myself off the reviewer list. I think setting up a time in the Calyx Monday meeting might be your best bet to get some eyes on this. |
nathanielnrn
commented
Jul 15, 2025
Comment on lines
-660
to
-666
| [[tests]] | ||
| name = "fud2 cocotb AXI-wrapped xecution" | ||
| paths = ["yxi/tests/axi/*/*-mem-vec-add-verilog.v"] | ||
| cmd = """ | ||
| fud2 {} --from verilog-noverify --to cocotb-axi --set sim.data=yxi/tests/axi/vectorized-add.data 2> /dev/null | ||
| """ | ||
|
|
There was a problem hiding this comment.
imo, this is the end goal test we want to eventually support because it tests end to end functionality.. Keeping everything up to date and having fud2 find a single path from .futil to .dat that goes through all the right steps (with multiple inputs) was the blocker on leaving this test without all the intermediate steps being tested for above it, iirc.
There was a problem hiding this comment.
Great. Point taken: the thing we need to revive here is a way to test end-to-end execution, which currently requires multiple fud2 invocations (because of fundamental fud2 limitations).
|
Great; thank you for this cleanup, @nathanielnrn! This seems like the right thing to do, and I've taken note of the need for a new testing strategy here. |
sampsyo
added a commit
that referenced
this pull request
Jul 25, 2025
Hey there, This PR is an attempt to salvage what I can from the last big thing I worked on for the lab. The original attempt at this is in closed PR #2267. A bit of context in [zulip](https://calyx.zulipchat.com/#narrow/channel/445339-calyx-yxi/topic/AXI.20Controller/near/528563512) too Broadly speaking this PR does 3 things: 1. This PR introduces an axi_controller_generator.py which is meant to create a subordinate adhering to Xilinx's [control spec](https://docs.amd.com/r/en-US/ug1393-vitis-application-acceleration/Control-Requirements-for-XRT-Managed-Kernels). There are definitely some issues with it, but I think this is a good starting point. 2. Adds some fud2 functions to go from verilog to a `.xo`. This is different from the existing `.futil` to `.xo` and is the intended way to create a `.xo` with the Calyx-AXI-wrappers. 3. Changes the cocotb test bench to play nicer with the AXI wrappers that conform with Xilinx expectations (they are prefixed with `m_axi`. Unfortunately to get things like the axi_controller to conditionally be added and build, the `fud2` work and `cocotb` changes needs to be included. --- This introduces a lot of changes that are not very well tested. This is me doing what I can to at least leave things in a state that gives some kind of context for when this again gets pick up by folks, and doesn't have the work that was done go completely to waste. A lot of things to note in this PR, in no particular order 1. As a reminder, there is the (most recently worked on) `dynamic_axi_generator.py`, which is intended to respond to external signals for data movement across AXI, and a more standard read-compute-write `axi_generator.py` which assumes that all data moved, then computed, then moved back. The changes to the cocotb test runner are breaking for the read-compute-write version, but I think most of the fixes are simple, prefixing the appropriate wires with `m_axi_` there. I just didn't want to add more to this PR. 2. Some of the changes to the wrappers are not well tested. I got some `x`s in a cocotb test for the dynamic generator, implying there are probably some new errors in the AXI controllers. 3. PR #2515 removed some testing for the wrappers because they were super tedious to work with and not very informative, something to be aware of. 5. I made sure that instructions in `xilinx.md` work, in that they successfully can create an xclbin. The correctness of the hardware created isn't guaranteed unfortunately. 6. Also made sure that the cocotb-axi-testbench works with the _dynamic_ AXI wrapper. --------- Co-authored-by: Adrian Sampson <[email protected]>
nathanielnrn
added a commit
that referenced
this pull request
Aug 8, 2025
This gives us a single runt test that goes from a calyx vector add to its expected output after being simulated with cocotb. Can replace a lot of what was removed in #2515
nathanielnrn
added a commit
that referenced
this pull request
Aug 8, 2025
This gives us a single runt test that goes from a calyx vector add to its expected output after being simulated with cocotb. Can replace a lot of what was removed in #2515
sampsyo
pushed a commit
that referenced
this pull request
Aug 8, 2025
This gives us a single runt test that goes from a calyx vector add to its expected output after being simulated with cocotb. This also changes some wire names in the compute-read-write/static AXI wrapper generator that aligns with what cocotb expects. Now the dynamic and static versions are in sync. Can replace a lot of what was removed in #2515. Pleasantly surprised to find that the static generator still simulates correctly
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR gets rid of the yxi-based wrapper runt tests mentioned in #2233.
These files tried to snapshot "steps" in the process of going from a calyx program to AXI based cocotb testing, but the process for updating was manual (updating snapshots one at a time then copying the new
*.expectto the correct*.file-suffix) and error prone. (Forgetting to update a*.file-suffixwould just leave the tests passing). Once work starts up on AXI again I do think something in the spirit of these tests would be useful to have. The cocotb axi tests working is a good starting point to making sure that the AXI wrappers are [at least partially] correct.With the changes introduced in #2516 being not well tested, I think it makes sense to get rid of these tests, as long as we want to merge those changes as well.
Some more context in zulip