push constant range validation doesn't check for static usage

[jeffbolznv]

The CTS test dEQP-VK.pipeline.push_constant.graphics_pipeline.overlap_2_shaders_vert_frag (and a few similarly named tests) generate validation errors like:

ERROR:  [ UNASSIGNED-CoreValidation-Shader-PushConstantNotAccessibleFromStage ] Object: VK_NULL_HANDLE (Type = 0) | Push constant range covering variable starting at offset 12 not accessible from stage VK_SHADER_STAGE_FRAGMENT_BIT (code 0xffffffff from Validation at Unknown:0)

The fragment shader is:

#version 450
layout(location = 0) in highp vec4 vtxColor;
layout(location = 0) out highp vec4 fragColor;
layout(push_constant) uniform Material
{
    layout(offset = 12) int dummy1;
    layout(offset = 16) vec4 dummy2;
    layout(offset = 32) vec4 color;
} matInst;
void main (void)
{
    fragColor = vtxColor;
    fragColor += matInst.color;
    fragColor = min(mod(fragColor, 2.0), 2.0 - mod(fragColor, 2.0));
}

which doesn't statically use dummy1. It looks like MarkAccessibleIds is supposed to handle static usage, but I guess it probably isn't digging down into the structure's members.

[jzulauf-lunarg]

@chrisforbes -- I reviewed this code back a bit and came to the same conclusion. Getting the needed data is down in the guts of the spirv walker, do you have time to grab this?

[chrisforbes]

Spec interpretation question first.

14.5.1 says this:

14.5.1. Push Constant Interface
The shader variables defined with a storage class of PushConstant that are statically used by the shader entry points for the pipeline define the push constant interface. They must be:

typed as OpTypeStruct,

identified with a Block decoration, and

laid out explicitly using the Offset, ArrayStride, and MatrixStride decorations as specified in Offset and Stride Assignment.

There must be no more than one push constant block statically used per shader entry point.

Each variable in a push constant block must be placed at an Offset such that the entire constant value is entirely contained within the VkPushConstantRange for each OpEntryPoint that uses it, and the stageFlags for that range must specify the appropriate VkShaderStageFlagBits for that stage. The Offset decoration for any variable in a push constant block must not cause the space required for that variable to extend outside the range [0, maxPushConstantsSize).

Any variable in a push constant block that is declared as an array must only be accessed with dynamically uniform indices.

I read this as defining the variables to be blocks (well, 0 or 1 blocks in a valid shader), and so that being the granularity at which static use is supposed to be evaluated, which we do already. This is also consistent with static use granularity for other things -- input/output interfaces, descriptor backed resources, etc.

I think we just say WAI?

[jzulauf-lunarg]

"Each variable in a push constant block" -- the "each" makes it seem that more than one variable could be in a push constant block, but I don't know how that would be expressed in SPIR-V

[chrisforbes]

Right, this is weirdly describing things both ways.

[chrisforbes]

OK, I would prefer a world where none of the static use rules required reaching into the guts of block types, which we /almost/ have.

[jzulauf-lunarg]

I don't think we can avoid it, though I know why you do.

[jeffbolznv]

I see what you mean. I will file a spec bug to clarify this, however I believe the intent is to look at static use of members, otherwise the whole structure always starts at offset 0 (it's just structure padding) and mostly defeats the purpose of having the push constant ranges at all.

[chrisforbes]

@jeffbolznv That doesn't seem quite true -- GLSL source language allows for layout(offset=N) decorations on the block members to get a particular layout.

[tklajnscek]

Am I right in assuming that with the 1.1.89 spec clarification the validation layer should only complain if the shader actually uses the members that are out of range? If so, we could remove a bunch of #ifdef bloat from our push constant declarations.

Any thoughts appreciated :)

[jeffbolznv]

@klajntib I think you are interpreting the spec update correctly, but I don't think the validation layers have been updated to reflect that.

[jzulauf-lunarg]

@chrisforbes -- you and I should re-triage this w.r.t. the new spec language. If we have to drill down to field scope in the static analysis it would be good to have a sense of "level of effort".

[mark-lunarg]

This issue has been fixed by @jeremy-lunarg.