DTolm / VkFFT

Vulkan/CUDA/HIP/OpenCL/Level Zero/Metal Fast Fourier Transform library
MIT License
1.48k stars 88 forks source link

Promise not to overwrite user's data #177

Open DejvBayer opened 3 weeks ago

DejvBayer commented 3 weeks ago

Hi,

I have noticed that most of the parameters passed to VkFFT are passed by address, for example:

void* buffer = ...;

VkFFTConfiguration config{};
config.buffer = &buffer;

The problem is that the buffer member is defined as void** and if I have a constant array of pointers, the compiler emits warning that I cast void* const* to void**. I would suggest changing all Type** members to Type* const* unless the pointers is modified by VkFFT. It is the same story for VkFFTLaunchParams. Example:

typedef struct {
        // ...
#if(VKFFT_BACKEND==0)
    VkBuffer* buffer;           // -> const VkBuffer* buffer;
    VkBuffer* tempBuffer;       // -> const VkBuffer* tempBuffer;
    VkBuffer* inputBuffer;      // -> const VkBuffer* inputBuffer;
    VkBuffer* outputBuffer;     // -> const VkBuffer* outputBuffer;
    VkBuffer* kernel;           // -> const VkBuffer* kernel;
#elif(VKFFT_BACKEND==1)
    void** buffer;              // -> void* const* buffer;
    void** tempBuffer;          // -> void* const* tempBuffer;
    void** inputBuffer;         // -> void* const* inputBuffer;
    void** outputBuffer;        // -> void* const* outputBuffer;
    void** kernel;              // -> void* const* kernel;
#elif(VKFFT_BACKEND==2)
    void** buffer;              // -> void* const* buffer;
    void** tempBuffer;          // -> void* const* tempBuffer;
    void** inputBuffer;         // -> void* const* inputBuffer;
    void** outputBuffer;        // -> void* const* outputBuffer;
    void** kernel;              // -> void* const* kernel;
#elif(VKFFT_BACKEND==3)
    cl_mem* buffer;             // -> const cl_mem* buffer;
    cl_mem* tempBuffer;         // -> const cl_mem* tempBuffer;
    cl_mem* inputBuffer;        // -> const cl_mem* inputBuffer;
    cl_mem* outputBuffer;       // -> const cl_mem* outputBuffer;
    cl_mem* kernel;             // -> const cl_mem* kernel;
#elif(VKFFT_BACKEND==4)
    void** buffer;              // -> void* const* buffer;
    void** tempBuffer;          // -> void* const* tempBuffer;
    void** inputBuffer;         // -> void* const* inputBuffer;
    void** outputBuffer;        // -> void* const* outputBuffer;
    void** kernel;              // -> void* const* kernel;
#elif(VKFFT_BACKEND==5)
    MTL::Buffer** buffer;       // -> MTL::Buffer* const* buffer;
    MTL::Buffer** tempBuffer;   // -> MTL::Buffer* const* tempBuffer;
    MTL::Buffer** inputBuffer;  // -> MTL::Buffer* const* inputBuffer;
    MTL::Buffer** outputBuffer; // -> MTL::Buffer* const* outputBuffer;
    MTL::Buffer** kernel;       // -> MTL::Buffer* const* kernel;
#endif
        // ...
} VkFFTConfiguration;

One more idea: for input buffers it may be convinient to use const void* const* to indicate that the input data are never overwritten (only for CUDA, HIP and LevelZero).

Thanks a lot!

David

DTolm commented 3 weeks ago

Hello,

This is a good idea, I have added it (to all user buffers except tempBuffer, this one is usually allocated and managed by VkFFT). However, it complicates things a bit. For example, cuLaunchKernel expects void as an argument (https://docs.nvidia.com/cuda/cuda-driver-api/group__CUDA__EXEC.html#group__CUDA__EXEC_1gb8f3dc3031b40da29d5f9a7139e52e15), so it will just move casting to void inside VkFFT.

Best regards, Dmitrii

DejvBayer commented 3 weeks ago

I know about it, however I believe this is a better solution. Thanks for the change!