Implicit int8->float cast adds Int8 capability to the shader without asking for GL_KHX_shader_explicit_arithmetic_types
zeux opened this issue · 12 comments
Given this shader:
#version 450
#extension GL_EXT_shader_16bit_storage: require
#extension GL_EXT_shader_8bit_storage: require
struct Vertex
{
float vx, vy, vz;
uint8_t nx, ny, nz, nw;
float tu, tv;
};
layout(binding = 0) readonly buffer Vertices
{
Vertex vertices[];
};
layout(location = 0) out vec4 color;
void main()
{
vec3 position = vec3(vertices[gl_VertexIndex].vx, vertices[gl_VertexIndex].vy, vertices[gl_VertexIndex].vz);
vec3 normal = vec3(vertices[gl_VertexIndex].nx, vertices[gl_VertexIndex].ny, vertices[gl_VertexIndex].nz) / 127.0 - 1.0;
vec2 texcoord = vec2(vertices[gl_VertexIndex].tu, vertices[gl_VertexIndex].tv);
gl_Position = vec4(position + vec3(0, 0, 0.5), 1.0);
color = vec4(normal * 0.5 + vec3(0.5), 1.0);
}
glslang compiles it without errors, but it adds Int8 capability which can't be used in Vulkan at this point in time which produces a shader that doesn't compile. Adding an explicit int8->int cast fixes the issue.
What's odd is that if I, say, copy the struct out like this:
Vertex v = vertices[gl_VertexIndex];
Then the code doesn't compile any more and glslang asks to add GL_KHX_shader_explicit_arithmetic_types extension - however, this doesn't happen for the implicit cast, which seems like a problem if I understand the intent of GL_KHX_shader_explicit_arithmetic_types - couldn't find the spec for this.
It is adding the capability because code generation chose to use OpConvertUToF, which is not covered by the 8-bit storage extension.
OpUConvert is covered by 8-bit storage, so it will depend on your path to change types, which you've influenced in your experiments.
I'll look at different code gen, but it's not quite clear exactly what should drive decisions here.
To be clear, there are two issues here:
- Implicit cast from uint8 -> float results in OpConvertUToF
- Int8 capability is silently added to SPIRV output
The first issue is somewhat annoying - it would be nice to have the compiler automatically convert uint8 to uint32 and only then cast to float unless Int8 is known to be available.
The second issue is more severe. Int8 capability is not supported by Vulkan atm in any way, which means that glslang shouldn't add it to shaders compiled for Vulkan under any circumstances. It would be fine to use functionality that doesn't require Int8 (e.g. cast to uint32). It would be fine to generate an error and require smth like GL_KHX_shader_explicit_arithmetic_types. But the status quo is that glslang can silently build a shader that can't be used in Vulkan - this is what would be great to fix first.
I agree with @johnkslang that it's not clear Glslang is doing the wrong thing here. We have to be more explicit for cases such as
void main() {
uint u =2 ;
int i = int(u); // This constructor is required.
}
So I think it would be ok to force a use of a constructor to describe intent clearly.
When I add int(...) around the uses of the .nx, .ny, .nz fields then it compiles without needing Int8 capability. That seems ok, and I would think that limitation could be documented in the GLSL extension.
I'm okay with having to be more explicit. I'm not okay with glslang compiling my GLSL shader, producing a Vulkan SPIRV binary, and this binary not being valid because Vulkan doesn't even have a way to provide Int8 capability. glslang shouldn't produce a binary that is declared invalid by validator.
The big picture
Vulkan doesn't even have a way to provide Int8 capability
Doesn't GL_KHX_shader_explicit_arithmetic_types need that?
One fundamental issue here is that instead of glslang doing (A), we mostly do (B):
(A) get an explicit list of capabilities to stay within from the coder, and code-gen within that constraint
(B) see what capabilities are apparently auto-requested by the GLSL, and request those of Vulkan
It is "only" mostly (B), because now, for issues like this, it seems we should instead look at what extensions were and were not requested and make decisions based on that. (Also "mostly", because we have a few command-line options now to describe the target, but nothing like getting to choose arbitrary subsets of capabilities.)
So, I basically agree with most of what everyone above is saying, but it is simply a complex situation, that's all I meant initially.
This specific issue
I somewhat think it is a flaw that the extension doesn't allow the OpConvertUToF, but for the moment that decision is already baked into the extension, validator, drivers, etc.
For now, we can decide we are not targeting Int8 based on the requested extensions, so glslang can use that "extra" information to not generate an OpConvertUToF, though I have not yet looked at how difficult that is (e.g., if a transform undoes something).
Doesn't GL_KHX_shader_explicit_arithmetic_types need that?
Yeah - it does. My understanding is that there's no Vulkan extension that provides Int8 capability so GL_KHX_shader_explicit_arithmetic_types isn't actually useful at this point in time in Vulkan.
So, that could change any time, and can't be a design point for glslang. Will consider the plan two notes up to base things on the set of extensions declared.
I found another related case where glslang outputs bogus code:
#extension GL_EXT_shader_8bit_storage: enable
#extension GL_EXT_shader_16bit_storage: enable
layout(binding = 0, set = 0) buffer a_layout {
int8_t i8;
uint8_t u8;
int16_t i16;
uint16_t u16;
};
layout(binding = 1, set = 0) buffer b_layout {
float f[];
};
void main() {
f[gl_GlobalInvocationID.x] = float(i8);
f[gl_GlobalInvocationID.x] = float(u8);
f[gl_GlobalInvocationID.x] = float(i16);
f[gl_GlobalInvocationID.x] = float(u16);
i8 = int8_t(f[gl_GlobalInvocationID.y]);
u8 = uint8_t(f[gl_GlobalInvocationID.y]);
i16 = int16_t(f[gl_GlobalInvocationID.y]);
u16 = uint16_t(f[gl_GlobalInvocationID.y]);
}
Note that it will turn into an illegal OpConvertUToF, OpConvertSToF, OpConvertFToU, or OpConvertFToS which is not permitted in the SPIR-V extension specifications.
I had a look at how to fix this in glslang, but I think I'd need to add a raft of new conversion operations so that in Intermediate.cpp I can create the conversion operations that will do the correct int8_t -> int32_t -> float casts.
I found another related case where glslang outputs bogus code:
The code is good code, but targets the wrong target. The fundamental (and huge) shift in request here is to switch from generating capabilities based on what's seen to constraining code gen based on what extensions were declared.
I think I'd need to add a raft of new conversion operations
Doesn't just staying within the allowed operations, given the declared extensions, work?
I'm more concerned that every level in the compilation stack that can gen/transform code, will need a description of the target, so that it knows what envelope of instructions it's allowed to play in.
The alternative to all this, at least for this case, is simply to allow the extra conversion instructions in the storage extensions.
The code is good code, but targets the wrong target. The fundamental (and huge) shift in request here is to switch from generating capabilities based on what's seen to constraining code gen based on what extensions were declared.
I don't understand your logic here - GLSL doesn't natively have 8-bit type support without declaring an extension to bring in support. In my example I did not declare any extension allowing full use of the 8-bit extension, only using the 8-bit & 16-bit storage extensions to allow them as storage types only. Fundamentally I believe GLSL should behave correctly based on what user extensions were declared.
Doesn't just staying within the allowed operations, given the declared extensions, work?
That would work - but we need to add frontend checks to ban the 'wrong' casts. I'm getting reports from lots of developers who are suffering from glslang including unsupported capabilities because they are expecting glslang to 'sort it out' for them. If you think it is easier to add in frontend sema to output errors for the bad casts I am all ears 😄
The alternative to all this, at least for this case, is simply to allow the extra conversion instructions in the storage extensions.
If you want to propose this should we take this issue internal and discuss it within the working group?
I don't understand your logic here - GLSL doesn't natively....
The point is to see this as two programming models: the programming model of the HLL and the programming model of the generated SPIR-V. HLSL has always done this; allowing things in the HLL that the shader model does not support, and expecting the compiler to bridge the gap.
Instead, GLSL has always had the target model directly reflected in the GLSL itself; there was just one model. But, now I think we're starting down a path of splitting to two models: having a choice of generating two different sets of instructions for the same source, one way is valid for one target, but not another.
If you want to propose this should we take this issue internal and discuss it within the working group?
Yes.
I am not sure when exactly this was fixed but with a recent glslangValidator the shader refuses to compile, and adding integer casts fixes that - at no point the KHX extension is being added which is exactly what I was hoping the behavior would be. So I'm closing this - thanks!