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

refactor(gpu): fix sample extraction when nth > 0 and keep input unchanged #1425

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

pdroalves
Copy link
Contributor

@pdroalves pdroalves commented Jul 25, 2024

This PR:

  • introduces a rust test for GPU's sample extraction,
  • fixes it when nth > 0,
  • refactors it so the input (the glwe) is not modified by the method.

We might also observe another performance improvement.

closes: https://github.com/zama-ai/tfhe-rs-internal/issues/671

PR content/description

Check-list:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Relevant issues are marked as resolved/closed, related issues are linked in the description
  • Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

@cla-bot cla-bot bot added the cla-signed label Jul 25, 2024
@pdroalves pdroalves force-pushed the pa/refactor/sampleextraction branch 3 times, most recently from a53a7b5 to d3ea654 Compare July 25, 2024 20:11
@pdroalves pdroalves force-pushed the pa/refactor/sampleextraction branch from d3ea654 to b2e01f6 Compare July 25, 2024 20:25
Copy link
Contributor

@agnesLeroy agnesLeroy left a comment

Choose a reason for hiding this comment

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

Hey @pdroalves! Thank you very much for this. I have some comments on the PR but mainly a question: shouldn't we apply sample extraction to a list of GLWE ciphertexts and output a list of LWE ciphertexts? That would be more efficient for the compression of a vector of inputs, which is what we'll need if I'm correct. Then you don't need to add types for CudaGlweCiphertext and CudaLweCiphertext, you can reuse the existing List types.

backends/tfhe-cuda-backend/cuda/include/functions.h Outdated Show resolved Hide resolved
tfhe/src/core_crypto/gpu/vec.rs Outdated Show resolved Hide resolved
tfhe/src/core_crypto/gpu/entities/lwe_ciphertext.rs Outdated Show resolved Hide resolved
@pdroalves pdroalves force-pushed the pa/refactor/sampleextraction branch 11 times, most recently from 249a7f0 to 40ee415 Compare July 29, 2024 13:58
Copy link
Contributor

@agnesLeroy agnesLeroy left a comment

Choose a reason for hiding this comment

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

Hey @pdroalves! Thanks a lot for this PR 🙂 I just have some small comments about it, after that it'll be good to go!

backends/tfhe-cuda-backend/cuda/src/crypto/ciphertext.cuh Outdated Show resolved Hide resolved
backends/tfhe-cuda-backend/src/cuda_bind.rs Show resolved Hide resolved
backends/tfhe-cuda-backend/cuda/include/ciphertext.h Outdated Show resolved Hide resolved
backends/tfhe-cuda-backend/cuda/src/crypto/ciphertext.cu Outdated Show resolved Hide resolved
tfhe/src/core_crypto/gpu/mod.rs Show resolved Hide resolved
tfhe/src/core_crypto/gpu/vec.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@agnesLeroy agnesLeroy left a comment

Choose a reason for hiding this comment

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

All good! Thanks!

@agnesLeroy agnesLeroy merged commit 3ba61c0 into main Aug 2, 2024
77 of 78 checks passed
@agnesLeroy agnesLeroy deleted the pa/refactor/sampleextraction branch August 2, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants