-
Notifications
You must be signed in to change notification settings - Fork 853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implicit int8->float cast adds Int8 capability to the shader without asking for GL_KHX_shader_explicit_arithmetic_types #1525
Comments
It is adding the capability because code generation chose to use
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:
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
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
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 issueI 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 |
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:
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. |
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.
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. |
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.
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 😄
If you want to propose this should we take this issue internal and discuss it within the working group? |
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.
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! |
Given this shader:
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:
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 ofGL_KHX_shader_explicit_arithmetic_types
- couldn't find the spec for this.The text was updated successfully, but these errors were encountered: