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

Unimplemented path in dead member elimination pass (spec constants) #3284

Closed
godlikepanos opened this issue Apr 8, 2020 · 2 comments · Fixed by #3289
Closed

Unimplemented path in dead member elimination pass (spec constants) #3284

godlikepanos opened this issue Apr 8, 2020 · 2 comments · Fixed by #3289
Assignees

Comments

@godlikepanos
Copy link

Hi,

I'm hitting an assertion when using -Os in spirv-opt:

spirv-opt: /xxx/source/opt/eliminate_dead_members_pass.cpp:298: spvtools::opt::EliminateDeadMembersPass::RemoveDeadMembers()::<lambda(spvtools::opt::Instruction*)>: Assertion `false && "Not yet implemented."' failed.

The shader is quite simple. It was compiled with glslang without opts:

#version 450 core

layout(constant_id = 0) const int c_0 = 1;
layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;

layout(set = 0, binding = 2, std140) buffer ss0_
{
	float u_s[c_0];
};

void main()
{
	u_s[gl_LocalInvocationID.x] = gl_LocalInvocationID.y + c_0;
}

SPIRV for it:

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %main "main" %gl_LocalInvocationID
               OpExecutionMode %main LocalSize 1 1 1
               OpSource GLSL 450
               OpName %main "main"
               OpName %c_0 "c_0"
               OpName %ss0_ "ss0_"
               OpMemberName %ss0_ 0 "u_s"
               OpName %_ ""
               OpName %gl_LocalInvocationID "gl_LocalInvocationID"
               OpName %c_0 "c"
               OpDecorate %c_0 SpecId 0
               OpDecorate %_arr_float_c_0 ArrayStride 16
               OpMemberDecorate %ss0_ 0 Offset 0
               OpDecorate %ss0_ BufferBlock
               OpDecorate %_ DescriptorSet 0
               OpDecorate %_ Binding 2
               OpDecorate %gl_LocalInvocationID BuiltIn LocalInvocationId
               OpDecorate %gl_WorkGroupSize BuiltIn WorkgroupSize
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
        %int = OpTypeInt 32 1
        %c_0 = OpSpecConstant %int 1
%_arr_float_c_0 = OpTypeArray %float %c_0
       %ss0_ = OpTypeStruct %_arr_float_c_0
%_ptr_Uniform_ss0_ = OpTypePointer Uniform %ss0_
          %_ = OpVariable %_ptr_Uniform_ss0_ Uniform
      %int_0 = OpConstant %int 0
       %uint = OpTypeInt 32 0
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
%gl_LocalInvocationID = OpVariable %_ptr_Input_v3uint Input
     %uint_0 = OpConstant %uint 0
%_ptr_Input_uint = OpTypePointer Input %uint
     %uint_1 = OpConstant %uint 1
         %25 = OpSpecConstantOp %uint IAdd %c_0 %uint_0
%_ptr_Uniform_float = OpTypePointer Uniform %float
%gl_WorkGroupSize = OpConstantComposite %v3uint %uint_1 %uint_1 %uint_1
       %main = OpFunction %void None %3
          %5 = OpLabel
         %20 = OpAccessChain %_ptr_Input_uint %gl_LocalInvocationID %uint_0
         %21 = OpLoad %uint %20
         %23 = OpAccessChain %_ptr_Input_uint %gl_LocalInvocationID %uint_1
         %24 = OpLoad %uint %23
         %26 = OpIAdd %uint %24 %25
         %27 = OpConvertUToF %float %26
         %29 = OpAccessChain %_ptr_Uniform_float %_ %int_0 %21
               OpStore %29 %27
               OpReturn
               OpFunctionEnd

Any plans on implementing this (@s-perron ?)? I'm afraid specialization constants have many issues in the optimizer (Another issue is #3282).

@godlikepanos godlikepanos changed the title Unimplemented path in dead member elimination pass Unimplemented path in dead member elimination pass (spec constants) Apr 8, 2020
@s-perron s-perron self-assigned this Apr 9, 2020
@s-perron
Copy link
Collaborator

s-perron commented Apr 9, 2020

That assert might have only been in there for me to check if it was used in the cases that I had, and was suppose to be removed. The type of spec constants should be marked as fully used when searching for the live member, so nothing needs to be done for spec constants.

I'll put up a PR that removes the assert, and does a little testing. @godlikepanos Could you just comment out the failing assert and see if that works for you?

@s-perron
Copy link
Collaborator

s-perron commented Apr 9, 2020

Now that I have looked at it closer I see what the problem was. This is for SpecConstantOp for operations that might extract a particular member. Those member must continue to exist. This will be fixed by adding an extra safety check in find live members. Specifics will be in the PR.

s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Apr 9, 2020
- Rewrite composite insert and extract operations on SpecConstnatOp.
- Leaves assert for Access chain instructions, which are only allowed
for kernels.
- Other operations do not require any extra code will no longer cause an
assert.

Fixes KhronosGroup#3284.
s-perron added a commit that referenced this issue Apr 9, 2020
* Handle more cases in dead member elim

- Rewrite composite insert and extract operations on SpecConstnatOp.
- Leaves assert for Access chain instructions, which are only allowed
for kernels.
- Other operations do not require any extra code will no longer cause an
assert.

Fixes #3284.
Fixes #3282.
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

Successfully merging a pull request may close this issue.

2 participants