The spirv-opt remove-duplicates pass makes a seemingly erroneous transformation of my shader

[StrayLightning]

I have run into a problem with spirv-opt since the release of Vulkan SDK 1.0.68.0. The remove-duplicates pass seems to be clobbering my shader. Prior to that release, things were fine. This may well be something that I am doing wrong, but I have been unable to determine what thus far.

spirv-opt from Vulkan SDK 1.0.68.0 self-identifies as:

SPIRV-Tools v2018.1-dev v2018.0-6-g9e19fc0

The original SPIR-V is constructed by glslangValidator from the same SDK.

Some relevant bits from my shader:

layout(set = 0, binding = 5, std140) uniform atmosphere_fragment_uniforms {
	float sun_radius;
};

layout(set = 1, binding = 12, std140) uniform planet_dynamic_uniforms {
	float lod_factor;
};

Before any optimisation the disassembly shows the following section:

OpName %planet_dynamic_uniforms "planet_dynamic_uniforms"
OpMemberName %planet_dynamic_uniforms 0 "lod_factor"

After running just the remove-duplicates pass this has changed to:

OpName %atmosphere_fragment_uniforms "planet_dynamic_uniforms"
OpMemberName %atmosphere_fragment_uniforms 0 "lod_factor"

There are other changes, but off the bat this seems odd.

Running the optimiser with every -O pass except for remove-duplicates generates SPIR-V that appears to work fine.

I tried using spirv-opt from the CI build from commit 6cd6e5e, but with no change in behaviour.

The following is a cut-down that generates the odd difference when running just remove-duplicates:

#version 450 core

layout(set = 1, binding = 12, std140) uniform planet_dynamic_uniforms {
	float lod_factor;
};

layout(set = 0, binding = 5, std140) uniform atmosphere_fragment_uniforms {
	float sun_radius;
};

layout(location = 0) out vec4 out_colour;

void main() {
	out_colour = vec4(1.0);
}

To demonstrate the difference:

glslangValidator -V -o cutdown.spv cutdown.frag
spirv-opt -o cutdown.opt.spv --remove-duplicates cutdown.spv
spirv-dis -o cutdown.s cutdown.spv
spirv-dis -o cutdown.opt.s cutdown.opt.spv
fc /l cutdown.s cutdown.opt.s

[StrayLightning]

I should add that the ultimate result is that performing reflection on the optimised SPIR-V results in incorrect set and binding point decorations for the uniform blocks. Which leads to the validation layers emitting for example:

ERRR: @[SC]: Shader uses descriptor slot 0.5 (used as type ptr to uniform struct of (float32)) but not declared in pipeline layout
ERRR: @[SC]: Shader uses descriptor slot 1.12 (used as type ptr to uniform struct of (float32)) but not declared in pipeline layout
ERRR: @[SC]: Shader uses descriptor slot 1.12 (used as type ptr to uniform struct of (float32)) but not declared in pipeline layout

[s-perron]

Thanks for opening the bug. We will look into it. Without looking in detail, the remove duplicates looks for types that are the same except for the name, and merges them into one. That is what you see happening. We will be in touch if we need more information.

[StrayLightning]

Thanks. That seems a bit odd to me if the layout specifiers are different, but perhaps I'm missing something.

[pierremoreau]

The layout specifiers are applied to the variables, not to the types, so merging the types should not impact those layout specifiers (and looking at the optimised SPIR-V binary, the layout specifiers are still properly applied).
Having multiple and different names applied to the same ID, is a bit confusing, but I don’t think that is responsible for your issue.

[pierremoreau]

Re-reading the specification, 1) we can’t have multiple times the same non-aggregate type, and 2) even though we can have multiple times the same aggregate type, they are still considered to be different types. So, if we disregard binaries produced during linking, there should never be duplicate types; removing duplicate types should probably be only run after linking multiple SPIR-V modules together.
I’ll look into making that change tomorrow.

[s-perron]

The reason we put this in is when we have two types that are that have the exact same layout and decorations, the TypeManager can get confused. Just because spec allows multiple isomorphic types, does it say it is wrong to replace one with the other? What is the semantic difference? It allows one object to be copied to another, but I do not know how it changes.

Obviously, we should remove the extra OpName instruction when we merge two types. I can get that done on Monday.

However, I'd like to understand the fundamental problem. Based on the error message, it looks like we are not removing a variable that is removed when we don't run remove duplicates. The error message seems to say that a descriptor slot is used, but it is not part of the pipeline layout. This looks like it is related to the decorations attached to the variables.

[pierremoreau]

I wonder if it’s not the reflection code which is getting confused by something in the binary without duplicates, because the errors are about both variables, not just one of them; I don’t see anything weird with that binary.

Obviously, we should remove the extra OpName instruction when we merge two types. I can get that done on Monday.

Just curious, are you planning to just keep the first one and remove all others?

[s-perron]

@pierremoreau I forget the code now, but at the point that you remove the decorations for one type, you should remove it's name as well. Whatever we do to remove the decorations should be replaced by a call to RemoveNamesAndDecorates. At least that is my guess.

[StrayLightning]

I am starting to suspect that the manner in which I am performing the SPIR-V reflection is incorrect.

[StrayLightning]

Then again, looking at it further, that transformation seems to make it impossible to perform the reflection. Is the solution to perform reflection prior to optimisation? I can imagine that other optimiser transformations would invalidate such reflection, though.

[s-perron]

I've been reading about this a little. @StrayLightning, can you run an experiment for me? Take the original shader that works, and manually change the spir-v to remove the OpName instructions. Let me know if that new shader still works.

If that works, can you replace the types on the OpVariable instructions for the uniform object so they have the same type.

I want to try to narrow down the change that actually makes a difference. It is that the the OpName for the types are used, or is it expecting every OpVariable instruction in the uniform storage class to have a different type.

[s-perron]

I don't think you are doing anything wrong. Do not do reflection before optimization. That can cause other problems, as you pointed out. I think the problem is a disconnect between what spirv-opt thinks is valid spir-v and what the reflection code expects.

[StrayLightning]

After commenting the following in the unoptimised SPIR-V:

           ; OpName %atmosphere_fragment_uniforms "atmosphere_fragment_uniforms"

The shader reflection now shows the uniform block with the name _56_57, which means that the application doesn't know anything about "atmosphere_fragment_uniforms" and fails.

If, instead of the above, I change:

    %__2 = OpVariable %_ptr_Uniform_planet_dynamic_uniforms Uniform

To:

    %__2 = OpVariable %_ptr_Uniform_atmosphere_fragment_uniforms Uniform

(I've done it this way around because of the order of things in the SPIR-V assembly: spirv-opt does the transform the other way around.)

In this case the reflection indicates two blocks with the name "atmosphere_fragment_uniforms" and the application can't reflect on "planet_dynamic_uniforms".

So the reflection seems to be reliant on both the OpName being present and the types of the uniform blocks, which seems a bit awkward from the point of view of de-duplicating types.

I hope this is of use.

[s-perron]

This is of use. At this point I have to say that the reflection code is wrong or there is a problem in the spec. The spir-v spec, referring to OpName instructions, says "This has no semantic impact and can safely be removed from a module." (https://www.khronos.org/registry/spir-v/specs/1.2/SPIRV.html#OpName).

The reflection code should not be relying on an OpName. This now goes beyond my knowledge. @dneto0 Do you now anything about this?

[pierremoreau]

I only had a quick look at the reflection code (plus I have never done any Rust before, so I might have misunderstood a few things in that code), but it seems that the reflection code is processing the SPIR-V (in some tree form), in order to discover the uniform blocks and other similar data, and you then use that knowledge to fill in the descriptor sets. Is this what is happening, or is there more to it? If not, then to achieve that, you shouldn’t need the names; Vulkan does not seem to ask for variable names either in its descriptor sets.

[StrayLightning]

The application lays out uniform buffers in memory to be vkMapMemory'd onto the GPU. In order to know what to place where, the application reflects the SPIR-V based on uniform block name and uniform offset (amongst other things.) It is true that the identifiers are not required to be handed on to Vulkan, but things like binding points are.

If something is truly dead and can be removed by optimisation of SPIR-V it makes sense that the application should be able to handle that case. In the case that the entity is alive, however, it's reasonable that the application can perform reflection on the shader, with the names used by the shader, in order to work out what goes where. Again, there might well be things I am doing wrong with the GLSL, Vulkan, or the reflection that is not valid.

There may well be better ways of achieving the same thing, but this is the model that I've come to from investigating what Vulkan developers say they do with their shader processing pipelines.

I am aware that this is probably consuming effort which might well be better spent elsewhere, and the problem might well be with how SPIRV-Cross is performing reflection, or how my application is using that information, or my shaders... My solution is to disable SPIR-V optimisation for now, and I'm happy with that. Many thanks for spending what time you already have looking at this. On the flip side, I am more than happy to provide additional information or dig into this in my own time (because it is interesting.)

It probably doesn't make any sense to try to understand the specifics of the reflection in my application, but in case you happened to be interested:

• Originally the reflection data was all manually encoded, which is a pain
• So I moved to parsing the output if glslangValidator, but that only goes so far
• So I built a C++ tool on top of SPIRV-Cross to perform a wider range of reflection, but using a similar output format to glslangValidator
• The spirv_cross Rust crate now provides enough access to SPIRV-Cross that I could replace the C++ with a chunk of Rust
• Currently the reflection data is read from the SPIR-V, written to disk, and then read back in (ugh)
• My intention is to remove this write-reflection-data-to-storage as it is no longer necessary

[pierremoreau]

Ah, I hadn’t seen you were using SPIRV-Cross for the reflection. I’ll try to get the whole stack running and trace down which part of the reflection is relying on OpName; I don’t know when I will have time to do that though, but hopefully before next week.

It is true that the identifiers are not required to be handed on to Vulkan, but things like binding points are.

Right, and the binding points and sets are still properly set AFAICS.

[StrayLightning]

Right, and the binding points and sets are still properly set AFAICS.

Though not under the same names as the un-optimised SPIR-V.

FWIW, I've been using such as the following to short-cut inspection of the transformed SPIR-V:

spirv-cross --dump-resources 5-atmosphere.frag.spv

[pierremoreau]

Though not under the same names as the un-optimised SPIR-V.

That is true, however if the reflection is iterating over the variables rather than the names, then it is not an issue (the binding point and set are attached to the ID of the variable).

Thanks for the short-cut.