Glavnokoman / vuh

Vulkan compute for people
https://glavnokoman.github.io/vuh
MIT License
346 stars 34 forks source link

allow to read SPIR-V from code segment #54

Closed federkamm closed 4 years ago

federkamm commented 4 years ago

I like to "compile" the spv files to C header files using xxd -i which gives me something like

/* shader.spv.h */
alignas(4) const unsigned char shader_spv[] = {
    0x4a, 0x01, /* ... */, 0x55
};

with the content of the spv file as plain C array which will be stored in the read-only "code section" of the linked program. Unfortunatelly, it seems to be impossible to get that array into a vector<char> without unnecessary allocations and copying it unnecessarily around. For this reason, I added the uint32_t size, const uint32_t* code constructor to the Program class. I'm not sure, this is the best solution to the problem but it is the one I can offer as pull request.

The "correct" way would be probably to use a std::span instead of a std::vector but which is only available in C++20. Another possibility would be probably to allow the user to construct their own ShaderModule and pass it as parameter to the constructor of Program. Variations to the approach in this pull request would be to use a different data type for the size (e.g. int or size_t) and/or code (e.g. uint8_t or char). For no particular reason I used the types (uint32_t and const uint32_t*) suggested by Vulkan-API.

federkamm commented 4 years ago

I think, it is "the right thing" to stick with const uint32_t* for the code. To my understanding, passing a (unsigned) char array instead of an uint32_t array is somewhat tricky and can easily result in undefined behaviour because of the strict aliasing rule. It seems to be not allowed to access a (unsigned) char array with an uint32_t, whereas the opposite (to access an uint32_t array by a character type) seems to be allowed. Technically, Vulkan accesses the elements of the array and one doesn't know if it accesses it through an uint32_t or an character type, so it is probably out of the scope of the C(++) standard, but I wouldn't bet on that. I decided to "compile" my SPIR-V shader to header files with a Makefile rule

%.spr.h: %.spr
-->|    echo '#include <stdint.h>' >$@
-->|    echo 'const uint32_t $*_spv[] = {' >>$@
-->|    hexdump -ve '"  0x%08X,\n"' $< >>$@
-->|    echo '};' >>$@

and include a SPIR-V file shader.spr it in the source code using this pull request like

#include "shader.spr.h"
/* ... */
auto program = Program<Specs, Params>(device, sizeof shader_spr, shader_spr);

where shader.spv.h looks like

#include <stdint.h>
const uint32_t shader_spv[] = {
  0x07230203,
  /* ... */
  0x000100FD,
  0x00010038,
};

I'm quite confident that this doesn't cause UB, whereas I'm not that confident using my previous unsigned char array approach or the vector<char> API.

federkamm commented 4 years ago

Yes, it probably should be size_t instead of uint32_t. I also realized, I didn't add the interface to the last specialization of class Program (non-empty specialization constants and empty push constants). I'll probably provide a pull request regarding this two issues, but probably not before September.