Skip to content
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: add example from paper for FHE. #237

Merged
merged 7 commits into from
Apr 17, 2024
Merged

feat: add example from paper for FHE. #237

merged 7 commits into from
Apr 17, 2024

Conversation

bollu
Copy link
Collaborator

@bollu bollu commented Apr 17, 2024

To make this example work, we need a definition of R.ofZ that is computable, and which is not agressively unfolded. This is ensured by our axiom trick.

Code peeled from pre-paper submission PR: #196.
Code stacked on: #234


Closes #196 (This PR fully integrates the features of #196 into main.)

To make this example work, we need a definition of `R.ofZ` that is
computable, and which is not agressively unfolded. This is ensured by
our `axiom` trick.

Code peeled from pre-paper submission PR: #196.
Code stacked on: #234

Co-authored-by: Andrés Goens <andres@goens.org>
@alexkeizer
Copy link
Collaborator

To clarify: the axiom trick is needed because of the reduction we do in the fhe_com elaborator?

@bollu
Copy link
Collaborator Author

bollu commented Apr 17, 2024

@alexkeizer that's right.

@bollu
Copy link
Collaborator Author

bollu commented Apr 17, 2024

@alexkeizer actually, on second thought, I think there are two places we agressively reduce: one is in simp_peephole/change_mlir_context, and one is at the fhe_com. I think it's the aggressiveness of simp_peephole / change_mlir_context that used to cause stuff to explode.

@bollu
Copy link
Collaborator Author

bollu commented Apr 17, 2024

@alexkeizer My hope is that the example is self-contained enough for someone (nudge nudge @tobiasgrosser ) to investigate if it's possible to eliminate the need for the axiom.

@alexkeizer
Copy link
Collaborator

Good to know: we could (should) change change_mlir_context to only reduce to a sort of transitive whnf, where it is of the form _ :: _ :: ... :: [] (with the cons being made explicit), but the elements don't have to be reduced. That would solve your issue as well, then?

@bollu
Copy link
Collaborator Author

bollu commented Apr 17, 2024

@alexkeizer that might, but I would not take my word for it. I'd have to play with the example again to remember precisely what caused me to need an axiom instead of just an implemented_by.

@alexkeizer
Copy link
Collaborator

alexkeizer commented Apr 17, 2024

I'd like to give it a try: reducing less aggressively seems like a win regardless of if it fixes this exact issue.

EDIT: I've attempted my suggestion in #238, but it does not seem to solve all of the problems here.
I suspect we do have to make the fhe_com less aggressively as well, which is a bit more involved.
I'll make a note of this in #232, but won't block the current PR any more on this

Copy link
Collaborator

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this is fine to merge after you've addressed my (minor) comments

SSA/Projects/FullyHomomorphicEncryption/Rewrites.lean Outdated Show resolved Hide resolved
SSA/Projects/FullyHomomorphicEncryption/Rewrites.lean Outdated Show resolved Hide resolved
SSA/Projects/FullyHomomorphicEncryption/Rewrites.lean Outdated Show resolved Hide resolved
@bollu bollu enabled auto-merge April 17, 2024 16:38
@bollu bollu added this pull request to the merge queue Apr 17, 2024
Merged via the queue into main with commit 2a33191 Apr 17, 2024
1 check passed
@bollu bollu deleted the fhe-add-paper-example branch April 17, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants