-
Notifications
You must be signed in to change notification settings - Fork 121
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
replace current fft with cufftdx #979
base: main
Are you sure you want to change the base?
Conversation
wee need to fix cmake, to automatically set CUDA_ARCH |
@@ -50,6 +50,7 @@ endif() | |||
|
|||
# Add OpenMP support | |||
find_package(OpenMP REQUIRED) | |||
find_package(mathdx REQUIRED COMPONENTS cufftdx CONFIG PATHS "/usr/include/nvidia/mathdx/22.11/") |
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 if this is the best solution, this seems platform specific?
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.
@IceTDrinker maybe you have an idea 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.
@bbarbakadze we should follow the indications from their documentation: https://docs.nvidia.com/cuda/cufftdx/installation.html. They recommend to have a copy of the header files in the tfhe-cuda-backend
it seems.
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.
But we have to be careful with licensing issues. We should leave this PR as a draft for now until this question is addressed.
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 are already platform dependent through linux and "worse" even Ubuntu officially ? As long as they follow the standard linux filesystem organization (linux standard base or something) I don't think it's too bad potentially, the thing I'm less sure about is the hardcoded version
License questions need to be adressed right away.
I don't see how having the headers in our project is a good idea/practice ? 🤔 could be a way for them to "hijack" code bases
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.
@bbarbakadze we should use the 24.01 version which is the latest.
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 I saw a thing in the Cmake to download the gtest thing, can it do the same for that lib ?
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.
Good idea, I'll look into it
// (int_fullprop_buffer<uint64_t> *)mem_ptr, static_cast<uint64_t *>(ksk), | ||
// bsk, lwe_dimension, glwe_dimension, polynomial_size, ks_base_log, | ||
// ks_level, pbs_base_log, pbs_level, grouping_factor, num_blocks); | ||
// break; |
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 we should delete those lines instead of commenting them out actually 🙂 Code will be cleaner and it's easy to add them back one day.
81028cd
to
3390925
Compare
Missing: update the CI to install cufftDx in all workflows. |
3390925
to
c45d264
Compare
PR content/description
491
Check-list: