-
Notifications
You must be signed in to change notification settings - Fork 130
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
Feat/synth extension #773
base: main
Are you sure you want to change the base?
Feat/synth extension #773
Conversation
Note: the last 2 commits on bit extracts will be merged in separate PR on main before this one |
5d887f6
to
a0c074e
Compare
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.
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.
a0c074e
to
2b368ed
Compare
lsb and tlu calls were not minimized
14c3890
to
7e8d75c
Compare
7e8d75c
to
ced1f37
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no, the expression is a verilog expression.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be in the merged PR right?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's truly temporary, we should use /tmp
.
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.
Same with others.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some blank lines would be good for readability.
|
||
|
||
@dataclass | ||
class Circuit: |
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 avoid duplication of existing class names. Let's go for VerilogCircuit
or something like that.
No description provided.