-
Notifications
You must be signed in to change notification settings - Fork 54
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
Openfhe: get mulDepth from LWE type #1189
Openfhe: get mulDepth from LWE type #1189
Conversation
e245811
to
a8a52f3
Compare
Rebased and Marked as ready since #1176 merged. |
} | ||
|
||
// CHECK: @complex_func | ||
// CHECK: @complex_func__generate_crypto_context | ||
// CHECK: mulDepth = 3 | ||
// CHECK: mulDepth = 2 |
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.
Just for confirmation: this is 2 now because of the inclusion of the optimal relin insertion pass (or rather, the maximum constraint within that pass that prevents going to depth 3).
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.
It is due to operation-balancer
.
Without them we get mulDepth 3 but with them (--secretize --wrap-generic --cse --canonicalize --operation-balancer --secret-insert-mgmt-bgv --secret-distribute-generic --secret-to-bgv=poly-mod-degree=32 --openfhe-configure-crypto-context=entry-function=complex_func
) we get mulDepth 2.
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 put the detailed flag in the test so we still get mulDepth = 3
. Otherwise it is too confusing.
Oh this one should wait until #1190 |
a8a52f3
to
f77a913
Compare
f77a913
to
ca9b464
Compare
Rebased and ready now. |
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.
LGTM👍, just a few questions/thoughts (that shouldn't stand in the way of merging this)
Related to #1145. This is a step towards a generic configuring crypto context pass, as the Lattigo backend also needs one.
Since we have RNS lowering for secret-to-, we can get mulDepth from LWE type.
Still marked as draft as the CKKS RNS lowering has not landed. All tests working when merged with #1176
Several changes
openfhe.mul
(implicit relin)configure_crypto_context_complex.mlir
removed scf.if and used arith.add instead, asarith.select
also does not have a lowering