KhronosGroup / SPIRV-Tools

Apache License 2.0
1.04k stars 545 forks source link

Correct std140 Uniform Buffer layout flagged as incorrect #1734

Closed danielmaciel closed 5 years ago

danielmaciel commented 6 years ago

As seen in: https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/252

The struct 'S' is a perfectly valid std140 layout struct S { mat4 m1; mat4 m2; mat4 m3; vec3 v; float f; }; But when trying to use it I get a warning:

UNASSIGNED-CoreValidation-Shader-InconsistentSpirv(ERROR / SPEC): msgNum: -1 - SPIR-V module not valid: Structure id 13 decorated as Block must follow standard uniform buffer layout rules: member 4 at offset 204 overlaps previous member ending at offset 207 OpFunctionEnd

Objects: 1 [0] 0x0, type: 0, name: (null)

Which is incorrect for this struct. The fact that vec3 has a 16-byte alignment does not mean it requires padding, it just means that it needs to be at at 16-byte alignment boundary. The extra padding may (and is encouraged to) be used by another 4-byte aligned variable (a float in this case).

danielmaciel commented 6 years ago

"By the rules of std140, each vec3 must start on a 16-byte boundary. But vec3 does not consume 16 bytes of storage; it only consumes 12. And since float can start on a 4-byte boundary, a vec3 followed by a float will take up 16 bytes."

asuonpaa commented 6 years ago

@danielmaciel have you tried this with the ToT SPIRV-Tools from this repo? Maybe this has been fixed upstream but Vulkan-ValidationLayers hasn't picked it up yet?

I tried to reproduce the issue using spirv-val compiled from the fresh repo. I was using the following GLSL code:

#version 450
layout (set = 0, binding = 0) uniform S
{
    mat4 m1;
    mat4 m2;
    mat4 m3;
    vec3 v;
    float f;
};

void main()
{
}

And then compiling and validating the results show no errors: glslangValidator -V test.vert && spirv-val vert.spv

Just to make sure I also added the resulting SPIR-V into validator test suite and it doesn't complain there either.

danielmaciel commented 6 years ago

Hi! I am using the latest code from git everywhere. Have you tried:

version 450

struct S { mat4 m1; mat4 m2; mat4 m3; vec3 v; float f; }; layout (set = 0, binding = 0) uniform UniformS { S s; }

I will check the output of the resulting SPIR-V at home using the human-friendly assembly and will paste it here. I am, however, using Google's glslc - which uses the same libs as glslangValidator - but it might be a bug specific to 'glslc'. I will check the output and let you know. Unfortunately I cannot get to it right away - but I will do it later today.

Best regards, Daniel

danielmaciel commented 6 years ago

I have attached a .spvasm which generate the warning. Please let me know if you want me to send the .spv file to some email (Github won't let me attach it here).

The struct which generates the warning is called 'RenderSetup'.

light.frag.spvasm.txt

asuonpaa commented 6 years ago

Thanks. I ran that file through spirv-as and spir-val and I'm still not seeing any warnings. That would indicate the problem is already fixed in the latest version.

baldurk commented 6 years ago

This bug seems to be present in the Vulkan SDK 1.1.82.0 from my testing:

$ which glslangValidator.exe
/c/VulkanSDK/1.1.82.0/Bin/glslangValidator.exe
$ which spirv-val
/c/VulkanSDK/1.1.82.0/Bin/spirv-val
$ cat a.frag
#version 450

layout (binding = 0) uniform UBO { vec3 a; float b; } ubo;
layout (location = 0) out vec4 col;

void main() { col = vec4(ubo.a, ubo.b); }
$ glslangValidator.exe -V a.frag -o a.spv
a.frag
$ spirv-val a.spv
error: line 46: Structure id 11 decorated as Block must follow standard uniform buffer layout rules: member 1 at offset 12 overlaps previous member ending at offset 15
  OpFunctionEnd
$ spirv-as light.frag.spvasm -o a.spv # From the attachment above
$ spirv-val a.spv
error: line 233: Structure id 11 decorated as Block must follow standard uniform buffer layout rules: member 4 at offset 204 overlaps previous member ending at offset 207
  OpFunctionEnd

This affects some of the shaders in Sascha Willems' samples (which is how I found it, via a fork which passes through spirv-opt/spirv-val during compilation).

asuonpaa commented 6 years ago

Tried the a.frag on my end. No errors.

Here's the tool versions I'm using:

spirv-val --version
SPIRV-Tools v2018.5-dev v2018.4-103-g3a20879

glslangValidator --version
Glslang Version: 6.7.2743
danielmaciel commented 6 years ago

Maybe you are using an older version?

spirv-val --version SPIRV-Tools v2018.5-dev v2018.4-110-gb6319c3a

glslangValidator --version Glslang Version: 7.8.2850

asuonpaa commented 6 years ago

Now I have exactly the same versions and I'm still not seeing any errors. Tried the a.frag above.

spirv-val --version
SPIRV-Tools v2018.5-dev v2018.4-110-gb6319c3

glslangValidator --version
Glslang Version: 7.8.2850
danielmaciel commented 6 years ago

Maybe @baldurk has some more info? I am a little bit out of ideas at the moment. I guess I could check the code that generates said wrarnings in SPIRV-tools at some point

danielmaciel commented 6 years ago

Indeed, glslang points to an older version of spirv-tools which has the bug. And Vulkan-ValidationLayers uses that. Runnig spirv-val from them produces the issue:

spirv-val --version SPIRV-Tools v2018.4-dev v2018.3-42-g9ecbcf5f

The latest spirv-val from upstream does not produce any output. I guess glslang should update their spirv-tools.

EDIT: Manually changing 'glslang/known_good.json' to point to the latest 'spirv-tools' solves the issue on the Vulkan-ValidationLayers.

greg-lunarg commented 6 years ago

I will update the glslang known-good posthaste.

karl-lunarg commented 6 years ago

LunarG has released SDK Version 1.1.82.1 to address this problem.

And the glslang known-good has been updated.

This issue can be closed.

danielmaciel commented 6 years ago

Thank you! :)

dneto0 commented 6 years ago

Thanks all for getting to the bottom of this. I think I may have introduced the bug in the layout verification, and then fixed it about a week later. We're looking at improving our processes to catch or prevent this kind of problem in the future.

dneto0 commented 5 years ago

I verified that light.frag.spvasm.txt validates correctly when validating to target-env vulkan1.1