VkFFT icon indicating copy to clipboard operation
VkFFT copied to clipboard

Promise not to overwrite user's data

Open davebayer opened this issue 1 year ago • 2 comments

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

davebayer avatar Jun 13 '24 06:06 davebayer

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

DTolm avatar Jun 13 '24 08:06 DTolm

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

davebayer avatar Jun 13 '24 08:06 davebayer