-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add a new test for the not cell. #8
Add a new test for the not cell. #8
Conversation
@gussmith23 - I added a test for mux + an implementation of the gates here. Notably, I added more |
@gussmith23 - Actually - do you want me to add |
Actually, what level of abstraction is egglog meant to run on? |
https://github.com/uwsampl/yosys/blob/5b4860cd73ddffa1eb95723192b32a0bd890ac47/backends/lakeroad/tests/simple-mux.sv - Also this file segfaults. If I do a:
Then do a:
It doesn't segfault. It seems like the main issue is there's an infinite loop between 1219 and 1249 in lakeroad.cc: |
It's fine to just add them as needed!
We should include reduction! Users don't have to use it; they can decide to "expand" their reduction explicitly if they want. But we should definitely include it still. Thank you!
I can take a look. |
Ok, I'll just add all the cells to this PR then. I shoudn't need them to theory(for hardware decomp) but we can add them for other use cases. |
@thiskappaisgrey Sorry, let me be clear: you don't have to add any cells you don't need. |
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.
See comments on the two test files for the requested changes!
In an earlier comment I described the two types of tests that this PR contains: vanilla Yosys tests in .ys scripts, and lit
/FileCheck
tests. I'm requesting that you convert these tests from lit
/FileCheck
-based tests into vanilla Yosys tests. The reason is this: eventually I'd like to get the Lakeroad backend merged into Yosys. Their tests look more like the "vanilla Yosys tests". I'm not sure they will want to merge in something that adds a dependency on lit
/FileCheck
. So it's best to have our tests in their test style.
Does that all make sense to you?
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.
Can you convert this to this style of test:
https://github.com/uwsampl/yosys/pull/4/files#diff-ab06010b226d815b770e84ccf1bfb5c06148206adb297c30259e07f012bee3cd
So it might look something like:
read_verilog <<EOF
module test(input [2:0] a, output [2:0] out);
// assign out = ~ a;
always_comb begin
case (a)
2'b01: out = 2'b10;
2'b00: out = 2'b10;
2'b10: out = 2'b00;
2'b11: out = 2'b01;
default: out = 2'b01;
endcase
end
endmodule
EOF
prep -top test
write_lakeroad
Furthermore -- I'm not sure you need pmuxtree
, proc
, opt
,memory
, techmap
, or abc
-- are those there on purpose? In fact, they're a pretty likely source of errors.
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.
Please see my comment on simple-mux.sv
and do the same thing to this test. Note that you won't be able to check the output any longer. That's OK for now.
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.
@gussmith23 do you just want these tests in backend/lakeroad/tests
?
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.
That's fine -- I'll end up shuffling them around once you add them.
@gussmith23 Just pushed. I included the code generated with |
I think if you want this merged into mainline yosys - all of the cells in https://yosyshq.readthedocs.io/projects/yosys/en/latest/CHAPTER_CellLib.html should be included (and also added to the |
That's a decision I'd like to make once I start talking to the Yosys people about merging (which won't be for a while). For now I'm trying to keep the language only as large as we need it. More features = more things to fix every time I make a major change, which isn't what I need! |
Thank you! I'll likely merge everything first and then figure out the bugs after. |
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.
Great work. Thanks for being so thorough in documenting the infinitely looping bug.
4ccc4ba
into
uwsampl:gussmith23/lakeroad-backend
Hm. I'm confused as to why it didn't make you a coauthor on the merge commit...I'm going to try to fix that in the repo settings |
Alright, thanks! |
For posterity: I was wrong, I merged this PR instead of squash-and-merging the PR. Hence your commits were merged whole-cloth into the other PR. I was expecting a single squashed coauthored commit (which is what happens when you squash-and-merge, like I did to #9). So you did get credit :) |
Haha, I don't really mind either way. Thank you though. |
Hi.
If you run this test by doing:
It fails because the
$not
cell is unimplemented. I don't know how to add it in the code but if I figure it out before you have the chance, I'll add the fix to this PR. Thanks.