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(gpu): Implement fft128 in cuda backend #1823

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

bbarbakadze
Copy link
Contributor

closes: please link all relevant issues

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

Copy link
Member

@guillermo-oyarzun guillermo-oyarzun left a comment

Choose a reason for hiding this comment

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

Good work man, it actually seems like a very complex thing to implement, so hats off!
Here comes my review of what I could get :)

}
}

// fn test_product<Scalar: UnsignedTorus>() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of all this comments?

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep all this comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably have the test using the functions you exposed for forward/inverse 128bit FFT @bbarbakadze, it would be good to have the same set of tests as the CPU.

@guillermo-oyarzun
Copy link
Member

there are many log files that should be erased

}
}

// fn test_product<Scalar: UnsignedTorus>() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep all this comments?

// println!("poly.len: {:?}", poly.len());
// println!("rust poly");
// for coefficient in poly.iter() {
// println!(
Copy link
Member

Choose a reason for hiding this comment

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

This could be removed too

@@ -0,0 +1,17 @@
#include <stdint.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be named fft128.h maybe, and we could place it in a new folder include/fft, instead of include/pbs, wdyt?

@@ -19,6 +19,7 @@ fn test_roundtrip<Scalar: UnsignedTorus>() {
let mut fourier_im0 = avec![0.0f64; fourier_size].into_boxed_slice();
let mut fourier_im1 = avec![0.0f64; fourier_size].into_boxed_slice();

println!("sizeof_scalar: {:?}", Scalar::BITS);
Copy link
Contributor

Choose a reason for hiding this comment

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

This print can be removed

@@ -62,6 +62,7 @@ fn main() {
"cuda/include/integer/integer.h",
"cuda/include/keyswitch.h",
"cuda/include/linear_algebra.h",
"cuda/include/pbs/fft.h",
Copy link
Contributor

Choose a reason for hiding this comment

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

The bindings.rs file should get updated in this PR, doesn't it get updated automatically when you build?

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