uwsampl / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
1 stars 1 forks source link

Add a new test for the not cell. #8

Closed thiskappaisgrey closed 10 months ago

thiskappaisgrey commented 11 months ago

Hi. If you run this test by doing:

cd lakeroad/tests/simple-not.sv
lit -v .

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.

thiskappaisgrey commented 11 months ago

@gussmith23 - I added a test for mux + an implementation of the gates here. Notably, I added more Ops to the lakeroad lang: ReduceAnd, ReduceOr and ReduceXor. I'll have to make a PR to add it to lakeroad later.

thiskappaisgrey commented 11 months ago

@gussmith23 - Actually - do you want me to add egg implementations for all of the cells you haven't implemented? I.e. Add Sub Mul.. The whole list of cell types is here: https://yosyshq.readthedocs.io/projects/yosys/en/latest/CHAPTER_CellLib.html . I'm just going to add the ones I need for now.

thiskappaisgrey commented 11 months ago

Actually, what level of abstraction is egglog meant to run on? ReduceOr, etc, should already be reduced when running techmap, so like, lakeroad doesn't need those ops. And for decompilation (my usecase) we only work with 1 bit logic-gates anyways, so I don't need them either. I think I'll just remove the other ops and just add NOT.

thiskappaisgrey commented 11 months ago

https://github.com/uwsampl/yosys/blob/5b4860cd73ddffa1eb95723192b32a0bd890ac47/backends/lakeroad/tests/simple-mux.sv - Also this file segfaults. If I do a:

write_verilog -noattr syn.v

Then do a:

read_verilog syn.v
write_lakeroad

It doesn't segfault.

It seems like the main issue is there's an infinite loop between 1219 and 1249 in lakeroad.cc: image (there's more to the gdb backtrace). I'm neither a C++ or a yosys expert but that might give you a clue as to what's causing the inifinite loop when I synthesize with noattr.

gussmith23 commented 11 months ago

@gussmith23 - Actually - do you want me to add egg implementations for all of the cells you haven't implemented? I.e. Add Sub Mul.. The whole list of cell types is here: https://yosyshq.readthedocs.io/projects/yosys/en/latest/CHAPTER_CellLib.html . I'm just going to add the ones I need for now.

It's fine to just add them as needed!

Actually, what level of abstraction is egglog meant to run on? ReduceOr, etc, should already be reduced when running techmap, so like, lakeroad doesn't need those ops. And for decompilation (my usecase) we only work with 1 bit logic-gates anyways, so I don't need them either. I think I'll just remove the other ops and just add NOT.

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!

https://github.com/uwsampl/yosys/blob/5b4860cd73ddffa1eb95723192b32a0bd890ac47/backends/lakeroad/tests/simple-mux.sv - Also this file segfaults. If I do a:

write_verilog -noattr syn.v

Then do a:

read_verilog syn.v
write_lakeroad

It doesn't segfault.

It seems like the main issue is there's an infinite loop between 1219 and 1249 in lakeroad.cc: image (there's more to the gdb backtrace). I'm neither a C++ or a yosys expert but that might give you a clue as to what's causing the inifinite loop when I synthesize with noattr.

I can take a look.

thiskappaisgrey commented 11 months ago

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.

gussmith23 commented 11 months ago

@thiskappaisgrey Sorry, let me be clear: you don't have to add any cells you don't need.

thiskappaisgrey commented 10 months ago

@gussmith23 Just pushed. I included the code generated with write_verilog -noattr in: https://github.com/uwsampl/yosys/pull/8/files#diff-b7a4af195ac6097870b28e761d31959c25b24ba5924ac818789ca20e7c598533 - where in this code, your backend doesn't infinite loop. I suspect the infinite loop has to do with the attributes.

thiskappaisgrey commented 10 months ago

: eventually I'd like to get the Lakeroad backend merged into Yosys.

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 lakeroad lang as well), unless u want people to do all of the passes first (which optimizes out the "high level" cells like $add or $reduceor, etc).

gussmith23 commented 10 months ago

: eventually I'd like to get the Lakeroad backend merged into Yosys.

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 lakeroad lang as well), unless u want people to do all of the passes first (which optimizes out the "high level" cells like $add or $reduceor, etc).

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!

gussmith23 commented 10 months ago

@gussmith23 Just pushed. I included the code generated with write_verilog -noattr in: https://github.com/uwsampl/yosys/pull/8/files#diff-b7a4af195ac6097870b28e761d31959c25b24ba5924ac818789ca20e7c598533 - where in this code, your backend doesn't infinite loop. I suspect the infinite loop has to do with the attributes.

Thank you! I'll likely merge everything first and then figure out the bugs after.

gussmith23 commented 10 months ago

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

thiskappaisgrey commented 10 months ago

Alright, thanks!

gussmith23 commented 10 months ago

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

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 :)

thiskappaisgrey commented 10 months ago

Haha, I don't really mind either way. Thank you though.