-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add external debug port during execution #1306
Conversation
One hack might be to pretty-print the MLIR op ( void __heir_debug(CryptoContextT cc, PrivateKeyT, sk, CiphertextT, ct);
void __heir_debug_with_print(CryptoContextT cc, PrivateKeyT, sk, CiphertextT, ct) {
printf("<op_str>");
__heir_debug(cc, sk, ct);
} |
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.
So this is awesome work!
I have a few nitpicks about the code, but otherwise I'd be happy to submit this as-is and leave any improvements for followup work.
One idea that comes to mind: have a default implementation that decrypts and prints. In particular, the helper function is expected to be provided in C++, but people using the frontend won't be able to provide that. Long term we may want a user-provided debugger callback in python using the OpenFHE python bindings, but that seems beyond the scope of this PR. As a middle-ground, we could have a pass option for add-debug-port
that specifies the (prefix of the) name of the debug function. If unset, then the compiler will emit a default implementation instead of just the external declaration.
For the numbered debug functions, I think this is an acceptable solution. Brainstorming on how to avoid this if we were forced to, while also keeping the definition at the lwe
level, I think we would need to add something like an OpaqueCiphertext
type to lwe
, as well as an lwe.cast
op that casts to and from these opaque types to regular ciphertext types. Then lowering to OpenFHE would trivially drop the casts and use OpenFHE's opaque type for everything. We'd have to be careful of how we use it, though, because casting from an opaque ciphertext type to a non-opaque type would be error-prone.
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 is awesome, thanks!!!
void __heir_debug(CryptoContextT cc, PrivateKeyT sk, CiphertextT ct) { | ||
PlaintextT ptxt; | ||
cc->Decrypt(sk, ct, &ptxt); | ||
ptxt->SetLength(8); | ||
std::cout << ptxt << std::endl; | ||
} |
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.
As Jeremy already said, it'd be amazing if there was a "default" debug function that gets emitted, toggled by some pass option.
Looking at this one, it seems like the only thing that's somewhat application dependent is the length, but I'd be happy to have a debug that just dumps the entire ptxt for now. Once we have layout encodings, the compiler will be able to know how many cleartext elements are actually in there anyway, then we can consider revisiting this.
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.
Looking at this one, it seems like the only thing that's somewhat application dependent is the length, but I'd be happy to have a debug that just dumps the entire ptxt for now.
If we delete the ptxt->SetLength(8);
, the debug output would be of size 315K (lots of output) and is quite unreadable.
As a middle-ground, we could have a pass option for add-debug-port that specifies the (prefix of the) name of the debug function. If unset, then the compiler will emit a default implementation instead of just the external declaration.
I think I am able to generate a default impl at heir-translate stage (at lwe stage, we do not have the printf op abstraction), and also use lwe underlying type to infer the pt length. One subtle thing is that, for dot_product we have two underlying type (tensor<8xi16> and i16) and we need use the "correct" underlying type to make the debug func work.
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 think it would be fair to merge this PR and work on a default improvement in followup, so as to ensure we get the other useful parts of this PR in (e.g., emitting func.call and private func.func)
302b861
to
ef472ba
Compare
Comments addressed. |
ef472ba
to
9afa3e8
Compare
CI failure unrelated to this PR.
This failure can also be seen in https://github.com/google/heir/actions/runs/12955968820/job/36141258147 |
9afa3e8
to
c8c2bd1
Compare
Dug around MLIR source and found |
} | ||
|
||
if (op.getNumResults() != 0) { | ||
os << variableNames->getNameForValue(op.getResult(0)) << " = "; |
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.
@ZenithalHourlyRate - I had cherry-picked this PR and was using it to work on a demo, and then realized that this doesn't print out the type. I had to replace with emitAutoAssignPrefix(result);
Merged as 9afa3e8 |
Partially address #1266. This PR illustrate we can print decryption result after each step, for debugging. Also, I use such interface for evaluating noise magnititude after each step, so this is part of the param selection support.
This PR does the following things
lwe-add-debug-port
pass that__heir_debug
after each HE oplwe-to-openfhe
to support these func declacation andfunc.call
Example
For dot_product_8, the execution result is
IR after
lwe-add-debug-port
IR after
lwe-to-openfhe
Then
heir-translate
would canonicalize all those debug callIn user file, user can specify what they want to debug, for example, printing decryption result
Discussion
.mlir
does not haveAnyType
, we have to use__heir_debug_#number
to address various LWE types. I could not think of better solution for now.