Feat/synth extension #773
Feat/synth extension #773
Conversation
|
Note: the last 2 commits on bit extracts will be merged in separate PR on main before this one |
rudy-6-4
force-pushed
the
feat/synth-extension
branch
2 times, most recently
from
5d887f6
to
a0c074e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: 14c3890 | Previous: aa3b4fa | Ratio |
|---|---|---|---|
v0 PBS table generation |
58646381 ns/iter (± 536829) |
58897620 ns/iter (± 468377) |
1.00 |
v0 PBS simulate dag table generation |
37026176 ns/iter (± 361634) |
36877868 ns/iter (± 372726) |
1.00 |
v0 WoP-PBS table generation |
66912243 ns/iter (± 870763) |
66672339 ns/iter (± 2268730) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
rudy-6-4
force-pushed
the
feat/synth-extension
branch
from
a0c074e
to
2b368ed
Compare
lsb and tlu calls were not minimized
rudy-6-4
force-pushed
the
feat/synth-extension
branch
2 times, most recently
from
14c3890
to
7e8d75c
Compare
rudy-6-4
force-pushed
the
feat/synth-extension
branch
from
7e8d75c
to
ced1f37
Compare
There was a problem hiding this comment.
Do we support synthesis outside direct circuits (i.e., with inputsets)?
Also, I don't think the actual implementation should happen on the graph level. I think it'd be better if we leave it to MLIR generation for a few reasons:
- We would be able to use it during conversion ourselves (with the current implementation, we can't do it easily since it happens during tracing)
- We would be able to support it without giving explicit types (since the actual conversion would happen after bounds measurement, we can measure the bounds, assign bit-widths, and then interact with verilog during MLIR generation to work on correct bit widths directly, without use needing to give to us explicitly)
What we can do instead is this:
- trace synthesis functionality into a single node:
%0 = x # EncryptedTensor<uint3, shape=(4,)> ∈ [0, 7]
%1 = synthesis.expression((a >= 0) ? a : 0)(%0) # EncryptedTensor<uint3, shape=(4,)> ∈ [0, 7]
return %1
- during bounds measurement, we would just evaluate the expression using verilog
- in mlir conversion, when we see a synthesis node, we would generate the actual operations we need to perform and convert them directly to MLIR.
- since we know the bit widths, user doesn't need to specify it explicitly
- and we can use it in our own conversion logic
I know it's a big change, but a very useful one IMO.
|
|
||
| import numpy as np | ||
|
|
||
| VERBOSE = False |
There was a problem hiding this comment.
This won't be in the merged PR right?
There was a problem hiding this comment.
Or at least be configurable with env variables?
There was a problem hiding this comment.
No but it's not meant for end user but dev.
| def yosys_script(abc_path, verilog_path, json_path, dot_file, no_clean_up=False): | ||
| """Generate `yosys` scripts.""" | ||
| no_cleanup = "-nocleanup -showtmp" if no_clean_up else "" | ||
| return f""" | ||
| echo on | ||
| read -sv {verilog_path}; | ||
| prep | ||
| techmap | ||
| log Synthesis with ABC: {abc_path} | ||
| abc {no_cleanup} -script {abc_path} | ||
| write_json {json_path} | ||
| """ + ( | ||
| "" if not dot_file else "show -stretch" | ||
| ) | ||
|
|
||
|
|
||
| def abc_script(lut_cost_path): | ||
| """Generate `abc` scripts.""" | ||
| return f""" | ||
| # & avoid a bug when cleaning tmp | ||
| read_lut {lut_cost_path} | ||
| print_lut | ||
| strash | ||
| &get -n | ||
| &fraig -x | ||
| &put | ||
| scorr | ||
| dc2 | ||
| dretime | ||
| strash | ||
| dch -f | ||
| if | ||
| mfs2 | ||
| #lutpack | ||
| """ |
There was a problem hiding this comment.
Some documentation would be nice 😅
There was a problem hiding this comment.
I have noclear idea what theses lines do, it's the default abc script (Ill check), I just made it explicit to be able to modify it.
| abc_file, lut_costs_file, yosys_file, verilog_file, verilog_content, json_file, dot_file=True | ||
| ): | ||
| """Run the yosys script using a subprocess based on the inputs/outpus files.""" | ||
| tmpdir_prefix = Path.home() / ".cache" / "concrete-python" / "synthesis" |
There was a problem hiding this comment.
If it's truly temporary, we should use /tmp.
There was a problem hiding this comment.
True, the intend is that at some point it will be a real cache, so we don't need to resynthetize the same stuff again and again.
frontends/concrete-python/concrete/fhe/extensions/synthesis/verilog_to_luts.py
Show resolved
Hide resolved
frontends/concrete-python/concrete/fhe/extensions/synthesis/verilog_to_luts.py
Show resolved
Hide resolved
| ) | ||
|
|
||
|
|
||
| YOSIS_EXE_NAME = "yowasp-yosys" |
There was a problem hiding this comment.
Thx I have corrected in the new PR
No description provided.