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.
| @@ -0,0 +1 @@ | |||
| http://yowasp.org/ | |||
There was a problem hiding this comment.
I'm not sure having a readme just for this is necessary, can you add this to the documentation of synthesis module instead?
frontends/concrete-python/concrete/fhe/extensions/synthesis/init.py
"""
Synthesis extension based on http://yowasp.org/.
"""
...| out = fhe.int5 | ||
| params = {"a": out} |
There was a problem hiding this comment.
I don't think out variable needs to be defined explicitly:
params = {"a": fhe.int5}is better IMO.
| params = {"a": out} | ||
| expression = "(a >= 0) ? a : 0" | ||
|
|
||
| relu = synth.verilog_expression(params, expression, out) |
There was a problem hiding this comment.
Let's stick to using fhe module for everything:
relu = fhe.synthesis.verilog_expression(params, expression, out)Also, verilog is an implementation detail, we might want to change it to some other backend, so maybe just:
relu = fhe.synthesis.expression(params, expression, out)is better, wdyt?
There was a problem hiding this comment.
yes and no, the expression is a verilog expression.
There was a problem hiding this comment.
Yeah, I guess. Okay then, but still, let's use fhe and fhe only.
| relu = synth.verilog_expression(params, expression, out) | ||
|
|
||
| @fhe.circuit({"a": "encrypted"}) | ||
| def circuit(a: out): |
There was a problem hiding this comment.
Ah, I see why you defined out, but it's weird because it's both the input type and the output type. Seeing a: out is confusing (i.e., is it the input type or the output type, can they be different, etc.)
| import concrete.fhe.extensions.synthesis as synth | ||
| from concrete import fhe |
There was a problem hiding this comment.
Let's stick with consistent usage like other extensions:
fhe.synthesis.xyz(...)Instead of importing things other than fhe.
|
|
||
| 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?
| 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 😅
| 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.
| abc_file.write(abc_script(lut_costs_file.name)) | ||
| abc_file.flush() | ||
| lut_costs_file.write(luts_spec_abc()) | ||
| lut_costs_file.flush() | ||
| verilog_file.write(verilog_content) | ||
| verilog_file.flush() | ||
| yosys_file.write(yosys_script(abc_file.name, verilog_file.name, json_file.name, dot_file)) | ||
| yosys_file.flush() |
There was a problem hiding this comment.
Some blank lines would be good for readability.
|
|
||
|
|
||
| @dataclass | ||
| class Circuit: |
There was a problem hiding this comment.
Please avoid duplication of existing class names. Let's go for VerilogCircuit or something like that.
No description provided.