replace current fft with cufftdx #979
Conversation
|
wee need to fix cmake, to automatically set CUDA_ARCH |
bbarbakadze
changed the title
| @@ -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.
I'm wondering if this is the best solution, this seems platform specific?
There was a problem hiding this comment.
@IceTDrinker maybe you have an idea here?
There was a problem hiding this comment.
@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.
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.
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.
@bbarbakadze we should use the 24.01 version which is the latest.
There was a problem hiding this comment.
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.
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.
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.
agnesLeroy
force-pushed
the
feat/gpu/cufftdx
branch
2 times, most recently
from
81028cd
to
3390925
Compare
|
Missing: update the CI to install cufftDx in all workflows. |
agnesLeroy
force-pushed
the
feat/gpu/cufftdx
branch
from
3390925
to
c45d264
Compare
PR content/description
491
Check-list: