search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.96k stars 819 forks source link

Bad SPIR-V generated for FindLSB, FindMSB, bit ops for 64-bit operands #2737

Open greg-lunarg opened 3 years ago

greg-lunarg commented 3 years ago

Under the extension GL_EXT_shader_explicit_arithmetic_types, 64-bit operands are allowed for FindLSB, FindMSB, bitfieldExtract, bitfieldInsert, bitCount, bitReverse.

Unfortunately, SPIR-V restricts its equivalents of FindLSB and FindMSB to 32-bit operands. Vulkan restricts SPIR-V's bit* equivalents to 32-bits.

We have confirmed that glslangValidator generates 64-bit FindLSB in SPIR-V, half-confirmed the same for bitCount, and suspect it does so for the others.

@gnl21 Do we want to add these restrictions to GLSL when generating SPIR-V?

We have anecdotally confirmed that two major IHVs generate bad results for 64-bit FindLSB and bitCount. So they likely will not see it as a reversal but rather as an encoding of existing restrictions in the hardware.

Or should glslangValidator support and generate code for 64-bit operands for these builtins?

It certainly would be nicer for users. And it probably would not be too hard to implement in the frontend. But it seems to violate the close-to-the-metal spirit of SPIR-V.

gnl21 commented 3 years ago

Support for these builtins in SPIR-V was discussed at Khronos here: spirv/SPIR-V#296.

It looks like these functions were not exposed in GLSL on 64-bit types except in the explicit types extension, so it was probably a mistake that they were included. I expect that the IHVs who are generating bad results for the 64-bit SPIR-V will probably have the same problems when using the GL extension, but it would be good to confirm that.

I don't really think restricting the operations when generating SPIR-V is the right answer. If they're actually supported in GLSL then we should make sure they're supported through the whole stack, but if their mention in the extension is a bug then we should just remove them unconditionally, not just when generating SPIR-V.

greg-lunarg commented 3 years ago

Thanks for your thoughts.

FYI, these functions are not explicitly mentioned in GL_EXT_shader_explicit_arithmetic_types, but ended up included in the extension by default since they are defined in the core spec with genIType and genUType, which are expanded to include 64-bit types by the extension.

Following your suggestion that we should restrict these to 32-bits if indeed 64-bit is not supported throughout the stack and across targets, the implied action would be to change GL_EXT_shader_explicit_arithmetic_types to explicitly define these functions to only support genI32Type or genU32Type operands.

I am not sure that experimental results will be sufficient to discern the intent of the IHVs regarding 64-bit support for these in GL. Rather, should we just ask them? The fact that 64-bit is not supported in either SPIR-V or Vulkan is a fairly strong hint that it would not be supported in non-SPIRV OGL either.

gnl21 commented 3 years ago

FYI, these functions are not explicitly mentioned in GL_EXT_shader_explicit_arithmetic_types, but ended up included in the extension by default since they are defined in the core spec with genIType and genUType, which are expanded to include 64-bit types by the extension.

Ah, that's interesting because the issue I linked mentioned that these functions were not supported in GLSL. I'll have a look at the extension and the expanding of genIType, it doesn't match my recollection of what was intended.

I am not sure that experimental results will be sufficient to discern the intent of the IHVs regarding 64-bit support for these in GL. Rather, should we just ask them? The fact that 64-bit is not supported in either SPIR-V or Vulkan is a fairly strong hint that it would not be supported in non-SPIRV OGL either.

Yes, I expect that it will not be supported. Any change will have to be approved by IHVs, so we'll definitely end up asking them. My thought about testing was only that if it didn't work then it would suggest that apps aren't relying on it. If it actually does work then perhaps we should add the support to SPIR-V instead.

sfricke-samsung commented 2 years ago

Related: There was not actual spirv-val support checking for 32-bit for the OpBit listed in VUID-StandaloneSpirv-Base-04781 https://github.com/KhronosGroup/SPIRV-Tools/pull/4758