protonscr

Use OpBranchConditional instead of OpSelect for ps_udiv in test_shader_instructions

vkd3dopen
HansKristian-Work/vkd3d-proton#2169 · opened 2024-10-22 by lukelmy · updated 2025-06-10 · 4 comments · github
Llukelmy 2024-10-22 github

For ps_udiv in test_shader_instructions, the spirv is:

         %45 = OpINotEqual %bool %43 %uint_0
         %47 = OpUDiv %uint %36 %43
         %48 = OpSelect %uint %45 %47 %uint_4294967295
         %49 = OpUMod %uint %36 %43
         %50 = OpSelect %uint %45 %49 %uint_4294967295

According to the spirv spec, it's an undefined behavior if Operand 2 is 0 for OpUDiv/SDiv and OpUMod/SMod.
And it can be avoided by using OpBranchConditional instead of OpSelect.

OpUDiv:
Unsigned-integer division of Operand 1 divided by Operand 2.
Result Type must be a scalar or vector of integer type, whose Signedness operand is 0.
The types of Operand 1 and Operand 2 both must be the same as Result Type.
Results are computed per component. Behavior is undefined if Operand 2 is 0.

BTW, if we convert the following glsl with glslang

    uint particleIn = particlesIn[gl_LocalInvocationIndex];
    particlesOut[gl_LocalInvocationIndex] = particleIn != 0 ? 65536 / particleIn : 4294967295;

the assembly result is

         %26 = OpLoad %uint %particleIn
         %29 = OpINotEqual %bool %26 %uint_0
               OpSelectionMerge %32 None
               OpBranchConditional %29 %31 %36
         %31 = OpLabel
         %34 = OpLoad %uint %particleIn
         %35 = OpUDiv %uint %uint_65536 %34
               OpStore %30 %35
               OpBranch %32
         %36 = OpLabel
               OpStore %30 %uint_4294967295
               OpBranch %32
         %32 = OpLabel

So it seems glslang would also use OpBranchConditional in this similar case.

Ddoitsujin maintainer 2024-10-22 github

Is this an actual issue for anything?

Moderately annoying that SPIR-V specifies UB here rather than just undefined result, but I'm not sure how much of a priority this should be and how many fossilize DBs we're going to invalidate by fixing this.

HHansKristian-Work maintainer 2024-10-22 github

Yes, unless this blows up on real-world implementation we care about, I don't see this as a priority. I speculate this is a spec bug and it should actually be undefined value instead.

Llukelmy 2024-11-06 github

Sorry for the late reply.

From following issues in spirv, it seems undefined behaviour is the decision made in Khronos.
https://gitlab.khronos.org/spirv/SPIR-V/-/issues/351: OpUDiv and OpUMod have undefined behavior if the divisor is 0.
https://gitlab.khronos.org/spirv/SPIR-V/-/issues/492: Overflow with OpSDiv, OpSRem, and OpSMod results in undefined behavior.

One reason is that division by 0 is undefined behaviour in LLVM and thus LLVM-based compiler may assume that it never happens.

This test failed with AMDVLK since LLVM optimizes the "divisor == 0" part.
https://github.com/llvm/llvm-project/issues/64240
https://github.com/llvm/llvm-project/pull/67282

It could be an issue if any game uses division by 0 with a graphics driver includes a LLVM-based compiler.
And it can be avoided by using OpBranchConditional instead of OpSelect

HHansKristian-Work maintainer 2025-06-10 github

Just randomly ran across this, but https://registry.khronos.org/vulkan/specs/latest/html/vkspec.html#fundamentals-general states that division by zero must not lead to termination.

EDIT: nvm, it's in 3.9.

Nothing extracted yet.