-
Notifications
You must be signed in to change notification settings - Fork 977
Add association-list-based helper functions into Rosette backend #5128
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
base: main
Are you sure you want to change the base?
Add association-list-based helper functions into Rosette backend #5128
Conversation
The existing access function isn't useful if we don't have access to the original names of the input/output/state signals. There may be a better way to do this, but it might require restructuring the SmtrStruct.
I am making one more change to this and then it will be ready for review. Converting to draft until then. |
Ready for review. |
Do you have an example of this? I remember they were backwards at one point so I had to reverse them, but they should be in the right order now: module \$fa (A, B, C, X, Y);
input [WIDTH-1:0] A, B, C;
output [WIDTH-1:0] X, Y; (struct _$fa_Inputs (A B C) #:transparent
; A (bitvector 1)
; B (bitvector 1)
; C (bitvector 1)
)
(struct _$fa_Outputs (X Y) #:transparent
; X (bitvector 1)
; Y (bitvector 1)
) If there is any rearranging of ports I would suspect it's in Yosys or the frontend rather than the Rosette backend.
As far as I understand Rosette/Racket, this should be possible with (e.g.)
I think you want to iterate over
I don't think adding the flag to the current tests is the right move, since that invites the possibility that the output stops being valid without the flag (though I realise this is already true for the |
yosys -p "read_verilog -sv DSP48E2.v; prep; clk2fflogic; write_functional_rosette -provides -assoc-list-helpers" This should produce a file with the ports reversed; that's true on my machine at least. If it's not true on yours...something extra weird is going on!
You're right that the accessors As an aside, I really do feel like this should be easier in Racket. I'm shocked that there's not a default way to look up a field on a struct if you know the field's name as a string. It could be that I'm totally wrong about this, though; maybe it's easy!
Thanks! Fixed.
Any concrete suggestions on where I should put the tests? Should I not write them in pytest? Should I add them as "functional tests"? Are those the same things as the pytest tests? |
Huh. It's not just backwards, it's alphabetised too, but only from the functional backends (and not just Rosette). I wonder if it's related to them being stored as a
Ah right, that is true, particularly with the renaming/escaping that happens when it creates a unique name. In that case it does make sense to have an output converter as well.
The Where you write the tests depends on if you prefer bash or python for scripting; if you prefer bash then you should put them in |
EDIT: I can get it to not alphabetise if I use iterator operator++() { index--; return *this; } Relatively straightforward to solve in c++20 with |
@KrystalDelusion I'm working on tests now, I will re-request you when it's ready! Feel free to unsubscribe for now. |
@KrystalDelusion I have updated this PR and it's ready for another review. I have also added testing by hooking into the existing tests, adding an arg which controls whether this feature is used. |
Failures from the last CI run look like this:
@KrystalDelusion or whoever it may concern: it seems like the functional tests are getting run under the Verific tests -- is that expected? It seems like one of two options:
2 seems more likely to me, namely because the failure is so basic -- ie that Racket isn't installed in the environment. That implies to me that the Verific testing workflow isn't meant to run the functional tests. |
I also believe it is the latter, and mentioned earlier that the smt and rkt tests were being skipped and we should enable them so uhh, that's probably on me for not checking if the prerequisites were installed 😛 |
what would you like me to do here?
…On Tue, Jul 1, 2025, 6:14 PM KrystalDelusion ***@***.***> wrote:
*KrystalDelusion* left a comment (YosysHQ/yosys#5128)
<#5128 (comment)>
I also believe it is the latter, and mentioned earlier that the smt and
rkt tests were being skipped and we should enable them so uhh, that's
probably on me for not checking if the prerequisites were installed 😛
—
Reply to this email directly, view it on GitHub
<#5128 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJFZANELOSCL44JYIPT2YD3GMW5ZAVCNFSM6AAAAAB5K7ZBF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMRWGAYTCMBVGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hopefully nothing, runner has just been updated and the test is re-running |
Use the unique cell name (cell type + parameters) for the vcd filename to avoid collisions when converting to fst.
Takes approx half the time, at least when testing locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a couple extra tests (separate from test_rkt
) could be added for checking failure states; like using the keyword helpers but then providing the wrong field names.
@@ -0,0 +1 @@ | |||
"""Python utilities for simulating Rosette code.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing it, this file isn't used anywhere (and has nothing in it).
if (!output_helper_name) | ||
log_error("if keyword helpers are enabled, both input and output helper names are expected"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for this error to occur?
w.pop(); | ||
} | ||
w.pop(); | ||
// Output struct keyword-based destructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes this a destructor?
@gussmith23 Can you take a look at the comments I left when you have a chance? Tests are running fine with parallelism enabled on CI now (shown on a test merge because the job can't be re-run anymore) |
What are the reasons/motivation for this change?
Currently, the Rosette code emitted by Yosys requires the user to ensure they correctly order module inputs when creating the "inputs" struct to be passed to the Rosette function generated for the module. This makes it difficult to use the backend in an automated way, and may lead to silent failures -- e.g., if we provide the right number of ports but in the wrong order, it won't be clear why our module isn't behaving as expected. (As an aside: even more confusingly, it seems like the backend may currently reverse the order of the ports!)
Similarly on the outputs side, it would be nice to have a way to access outputs via their name.
This change adds an optional helper functions (guarded by a flag) that can construct inputs/convert outputs to/from string-keyed association lists. Users can thus explicitly specify and access each input/output port by name. This allows users to be more confident that they are constructing the input struct correctly, and allows users to more easily access outputs by name.
Explain how this is achieved.
Two new Rosette/Racket functions are (optionally) written out. One wraps over the current inputs struct constructor, taking a string-keyed association list mapping input port names to values. The other takes an outputs struct and converts it into a string-keyed association list mapping output port name strings to values.
If applicable, please suggest to reviewers how they can test the change.
I would appreciate feedback on how to test. I see that the Python tests currently run the generated Racket files. Here are two options:
-keyword-constructor
flag in the existing tests, which will not actually cause the keyword constructors to be used, but it will cause them to be generated. This will at least test that we do not have syntax errors.Testing
I test this PR by hooking into the existing tests for the Rosette backend. Essentially, for each test of the Rosette backend, there are now two tests: one that doesn't use the association list helpers (ie, what the tests used to do) and a test that does use the helpers. This is achieved via pytest's
parametrize
. This effectively doubles the number of Rosette tests, which may not be wanted if testing time is an issue.TODO
access_by_base_name
feels unnecessary, butaccess
doesn't work for my purposes because I can't actually get the IdString of the field name.