Skip to content
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

Prototype a new CLI #904

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Sep 5, 2023

Demo:

$ cat pyproject.toml
[project]
name = "efbutils"
version = "0"

requires-python = "~=3.8"
dependencies = [
  "amaranth @ file:///${PROJECT_ROOT}/../amaranth"
]

[build-system]
requires = ["pdm-backend"]
build-backend = "pdm.backend"

[tool.pdm.build]
includes = ["efbutils/"]
$ cat Makefile
-include page_buffer.v.d
page_buffer.v:
        pdm run amaranth generate efbutils.ufm.reader.page_buffer:PageBuffer verilog -v $@ -d [email protected]
$ make page_buffer.v
pdm run amaranth generate efbutils.ufm.reader.page_buffer:PageBuffer verilog -v page_buffer.v -d page_buffer.v.d
$ make page_buffer.v
make: 'page_buffer.v' is up to date.
$ touch efbutils/__init__.py
$ make page_buffer.v
pdm run amaranth generate efbutils.ufm.reader.page_buffer:PageBuffer verilog -v page_buffer.v -d page_buffer.v.d

@whitequark whitequark changed the title amaranth_cli: prototype Prototype a new CLI Sep 5, 2023
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #904 (1fd9388) into main (d32ca04) will decrease coverage by 0.02%.
Report is 15 commits behind head on main.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #904      +/-   ##
==========================================
- Coverage   83.75%   83.74%   -0.02%     
==========================================
  Files          54       54              
  Lines        7678     7684       +6     
  Branches     1867     1869       +2     
==========================================
+ Hits         6431     6435       +4     
- Misses       1048     1049       +1     
- Partials      199      200       +1     
Files Coverage Δ
amaranth/back/rtlil.py 77.30% <100.00%> (-0.04%) ⬇️
amaranth/lib/fifo.py 93.71% <87.50%> (-0.37%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@whitequark whitequark force-pushed the amaranth-cli branch 4 times, most recently from bac83ad to 617fc9a Compare September 5, 2023 22:26
amaranth_cli/__init__.py Outdated Show resolved Hide resolved
# Generate Verilog file.
if args.verilog_file:
from amaranth.back import verilog
args.verilog_file.write(verilog.convert(component))
Copy link
Contributor

@cr1901 cr1901 Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the verilog.convert invocation should include name field (either as an input arg to main or maybe derive it from the class name? I prefer the former slightly to avoid accidental conflicts with primitive names in the Verilog synthesizer). In projects where you generate multiple Verilog modules from Amaranth, chances are the synthesizer is not going to like them all named top.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that you'll still get duplicate names for the submodules in that case, so it's not enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, per IRC, that this is a problem that needs to be solved. For the purposes of testing/finishing the refactor of my code for the time being, would you be willing to expose top-level module renaming from the CLI? It would allow me to write the FuseSoC generation in terms of the Amaranth CLI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try the updated version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This correctly creates the hiearchy:

pdm run amaranth generate efbutils.ufm.reader.demo:Demo -p num_leds 2         -p efb_config '{ "dev_density": "7000L", "wb_clk_freq": 12.08 }' -p ufm_config '{ "init_mem": null, "zero_mem": true, "start_page": 0, "num_pages": 2046}' verilog -v - | grep "module "
module top(rx, tx, leds);
module \top.efb (bus__adr, bus__cyc, bus__dat_r, bus__dat_w, bus__irq, bus__stb, bus__we, rst, clk, bus__ack);
module \top.por (clk, por_clk, rst);
module \top.reader (bus__data, bus__read_en, bus__valid, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__irq, efb__stb, efb__we, rst, clk, bus__addr);
module \top.reader.pagemod (clk, seq__data, seq__addr, seq__stb, seq__ack, rand__data, rand__addr, rand__read_en, rand__valid, rst);
module \top.reader.streammod (clk, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__irq, efb__stb, efb__we, stream__data, stream__addr, stream__stb, stream__ack, stream__ready, rst);
module \top.reader.streammod.seqmod (clk, ctl__cmd, ctl__data_len, ctl__done, ctl__op_len, ctl__rd__data, ctl__rd__stb, ctl__req, ctl__wr__data, ctl__xfer_is_wr, efb__ack, efb__adr, efb__cyc, efb__dat_r, efb__dat_w, efb__stb, efb__we, rst);
module \top.uart (i, data, ack, rdy, rst, clk, o);
module \top.uart.rx (rst, clk, divisor, i);
module \top.uart.tx (data, ack, rdy, rst, clk, divisor, o);
module \top.wait (rst, clk, stb);

Once I'm able to rename the top prefix (probably via an -n [NAME] argument to the CLI?), I can finish the refactor, and wait for other ppl to test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try the updated version.

The attribute sees essentially no use and the information is much
better served by putting it in the module name. In addition this
means that the entire tree can be renamed simply by renaming the top
module.

Tools like GTKWave show the names of the instances, not the modules,
so they are not affected by the longer names.
raise argparse.ArgumentTypeError(f"'{qual_name}:{mod_name}' refers to an object that is not elaboratable")
return obj
else:
raise argparse.ArgumentTypeError(f"{reference!r} is not a Python object reference")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to realize that by "object reference", the error means "your input argument fails to match a regex", rather than "Python couldn't find an object with that name" (which is what the ImportError exception handler is for).

Perhaps this else branch could have "expected format is path.to.mod:Obj" or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this could be improved!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to submit a PR against a PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in the coming days (my response time will be sporadic until possibly this Monday afternoon).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is done: whitequark#1

@cr1901
Copy link
Contributor

cr1901 commented Oct 6, 2023

I'm quite happy with this PR at this point. But I don't want it merged before other people have tested it. Any chance we could bring it up at the next meeting to let other ppl know to try it out and report issues as a "final comment period"?

@whitequark
Copy link
Member Author

Any chance we could bring it up at the next meeting to let other ppl know to try it out and report issues as a "final comment period"?

It's still a draft in my thoughts--I just prototyped it for you. Merging it needs an RFC first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants