Skip to content
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

Use OpBranchConditional instead of OpSelect for ps_udiv in test_shader_instructions #2169

Open
lukelmy opened this issue Oct 22, 2024 · 2 comments

Comments

@lukelmy
Copy link

lukelmy commented Oct 22, 2024

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.

@doitsujin
Copy link
Collaborator

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.

@HansKristian-Work
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants