-
Notifications
You must be signed in to change notification settings - Fork 133
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
Change Partition Operation #858
base: main
Are you sure you want to change the base?
Conversation
0c87d72
to
e651fc9
Compare
|
||
let arguments = (ins FHE_AnyEncryptedInteger:$input); | ||
let results = (outs FHE_AnyEncryptedInteger); | ||
let hasVerifier = 1; |
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.
For the partition attribute, can we have strings ? like the name of the partition.
With attribute name like src_partition, dst_partition.
With at least one of the two should be here.
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.
and who's supposed to set those names?
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 I'm ok with rudy, we need to known which are the source / target partitions else the optimization cannot known which part of the dag should be assigned to an already parametrized partition (the tfhe-rs one in our case). The frontend and/or the user could give this information.
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 wondering also how we should give the partition(s) parameters, any ideas?
@@ -2420,6 +2420,24 @@ struct BroadcastToLinalgGeneric | |||
}; | |||
}; | |||
|
|||
// This operation should be used by the optimizer in multi-parameters, then | |||
// removed. Its presence may indicate that mono-parameters might have been | |||
// used. This patterns just hint for a potential fix. |
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.
Removed or replaced by a conversion keyswitch. But isn't it done later during parametrization ? (inserting conversion keyswitch).
Ok it looks like the parametrization transform is named Optimizer.cpp.
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.
Optimizer.Cpp currently removes it without any effect. We can change its behavior once it's really plugged with the optimizer
|
||
let arguments = (ins FHE_AnyEncryptedInteger:$input); | ||
let results = (outs FHE_AnyEncryptedInteger); | ||
let hasVerifier = 1; |
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 I'm ok with rudy, we need to known which are the source / target partitions else the optimization cannot known which part of the dag should be assigned to an already parametrized partition (the tfhe-rs one in our case). The frontend and/or the user could give this information.
|
||
let arguments = (ins FHE_AnyEncryptedInteger:$input); | ||
let results = (outs FHE_AnyEncryptedInteger); | ||
let hasVerifier = 1; |
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 wondering also how we should give the partition(s) parameters, any ideas?
@@ -56,6 +56,19 @@ struct OptimizerPartitionFrontierMaterializationPass | |||
mlir::func::FuncOp func = this->getOperation(); | |||
|
|||
func.walk([&](mlir::Operation *producer) { | |||
mlir::IRRewriter rewriter(producer->getContext()); | |||
|
|||
// Remove the change_partition op. |
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 understand the TODO? Where is the parameters in the op?
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.
They are currently set in the frontend only, they should be forwarded to the op in a future commit
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 a few suggestions on the Python side, looks good 👍
frontends/concrete-python/concrete/fhe/compilation/module_compiler.py
Outdated
Show resolved
Hide resolved
frontends/concrete-python/concrete/fhe/compilation/module_compiler.py
Outdated
Show resolved
Hide resolved
mypy was complaining hint was a module
77ce401
to
035bd63
Compare
we are checking if the selected parameter selection can handle tfhers integers
035bd63
to
be92025
Compare
compilers/concrete-compiler/compiler/include/concretelang/Dialect/FHE/IR/FHEOps.h
Show resolved
Hide resolved
let summary = "An attribute representing a partition."; | ||
|
||
let parameters = (ins | ||
"int":$lweDim, |
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 actually not sure that all those attributes should be usefull, especially lweNoiseDistrStdDev, lweNoiseDistrStdDev and msgModulus, carryModulus which are encryption and encoding parameters for radix encoding and at the compiler level we should don't care of that.
But we can keep it like this then prune later.
@rudy-6-4 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.
We need at least the macro parameters that gives the size of the ciphertext and the noise level (either as manp or as a float). Others will not be used by the optimizer since they could be blocking. E.g. even for PBS parameter of this tfhe.rs partition, concrete can have its own parameters (using the same key as tfhers).
"int":$maxNoiseLevel, | ||
"FloatAttr":$log2PFail, | ||
"bool": $bigEncryptionKey, | ||
DefaultValuedParameter<"int", "-1">: $ciphertextModulus |
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.
Is that possible to have verifier on the attr? I guess we should as example not support for ciphertextModulus != 64 at least at the beginning.
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 it's possible AFAIK
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.
hasVerifier doesn't seem to be supported at least
If necessary, it keyswitch the ciphertext to a different key having a different set of parameters than the original one. | ||
|
||
Example: | ||
```mlir |
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.
Can you update the example with src and dst example?
|
||
let summary = "An attribute representing a partition."; | ||
|
||
let parameters = (ins |
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.
Missing the partition name
No description provided.