Allow passing readonly SSBO members as "in" parameters

[jeffbolznv]

In KhronosGroup/glslang#1870, glslang was changed to reject passing a readonly buffer member to a function if the formal parameter is not qualified as readonly. The spec language is:

Variables qualified with coherent, volatile, readonly, or writeonly may not be passed to functions whose formal parameters lack such qualifiers. (See section 6.1 “Function Definitions” for more detail on function calling.)

This made sense when image_load_store was added to GLSL (before SSBOs), since any image variable is a reference to memory. It was also a convenient way to say "you can't pass a readonly image to imageStore()." Then when SSBOs came along, they were added to this section by just saying "it's the same as with images":

The memory qualifiers coherent, volatile, restrict, readonly, and writeonly may be used in the declaration of buffer variables (i.e., members of shader storage blocks). When a buffer variable is declared with a memory qualifier, the behavior specified for memory accesses involving image variables described above applies identically to memory accesses involving that buffer variable.

But consider the following example using a readonly buffer:

layout(set = 0, binding = 0, std430) readonly restrict buffer A {
    float b;
} a;

void main()
{
   round(a.b);
}

According to this spec language, it is illegal to pass a.b to round() because a.b is readonly (inherited from its parent) and round's formal parameter is not qualified as readonly. However, round's formal parameter is implicitly in, and IMO it should be valid to pass a readonly buffer member as an in parameter. in is defined as "The keyword in is used as a qualifier to denote a parameter is to be copied in, but not copied out."

My mental model for this is something like "Reading a readonly buffer member returns an rvalue. values passed to an in parameter must be rvalues. Therefore it is valid to pass a readonly buffer member to an in parameter."

I propose we relax the GLSL spec to allow passing readonly buffer members to formal parameters qualified with in. To be consistent, it probably also makes sense to allow passing writeonly buffer members to formal parameters qualified with out.

[johnkslang]

GLSL was defined with copy-in/copy-out semantics, to unambiguously avoid all such problems, but then some objects got treated like references or handles to other things.

I think we need to be more clear when something is acting on a copy and when it is acting on the original.

[johnkslang]

I also think it is safer to revert the recent change until we have this fully sorted out.

Edit: The change was very syntactic, equating two syntaxes, so the underlying issue is the same either way.

The change just made these two the same:

// 1
layout(set = 0, binding = 0, std430) readonly restrict buffer A {
    float b;
} a;

// 2
layout(set = 0, binding = 0, std430) restrict buffer A {
    readonly float b;
} a;

Based on this language from the spec:

memory qualifiers may also be used in the declaration of shader storage blocks. When a
block declaration is qualified with a memory qualifier, it is as if all of its members were declared with the same memory qualifier.

[alegal-arm]

Is there anything holding back the revert? I'm afraid that we wouldn't be able to get any recent glslang changes into the CTS until this issue is resolved.

[johnkslang]

The revert does not change the fundamental problem the issue is about.

Do you want glslang to relax the read-only thing in general, not just for a failed inheritance?

[alegal-arm]

I understand that the revert doesn't solve the fundamental problem but I'm trying to solve practical problem of not being able to update glslang in the CTS. New restrictions break shader compilation.

Personally, I would prefer to relax the read-only thing in general but I assume it would require some change in the specification.

[johnkslang]

When the thing passed is effectively a pointer or handle to something else, the readonly is really a modifier on what is pointed to. Passing a readonly handle to a non-readonly in parameter that then goes and updates what was pointed to needs to be disallowed.

My mental model for this is something like "Reading a readonly buffer member returns an rvalue. values passed to an in parameter must be rvalues. Therefore it is valid to pass a readonly buffer member to an in parameter."

I think that works only because members don't happen to (currently) be pointers to other things, while the real distinguishing characteristic should be that it is either:

A. Essentially a pass-by-reference (or pointer, or handle, whatever the term of the moment is). Includes atomic counters, anything atomic operations act on, buffers as a whole, and textures/samplers/images (and futuristically interpolants that interpolate*() functions might be applied to) .

or

B. Everything else; normal stuff, where the entire object in question is [semantically] copied in and copied out, such that the function is operating only on the copy.

For B, readonly should be ignored when passing to in and inout and writeonly should be ignored when passed to out, as Jeff said, I'm just slicing it as A vs. B rather than block member vs. not block member.

I think glslang should move now in that direction as it is much closer (if not exactly) to spec. intent, and while it would technically be a spec. bug, the previous state of glslang for this was also a spec. bug, and adhering to the spec. is clearly wrong and breaks real things.

[johnkslang]

I pushed KhronosGroup/glslang@92f5afd as the least of evils until we have agreement on correct approach.

[pdaniell-nv]

We discussed this in the OpenGL/ES meeting today. @johnkslang agreed to propose a spec change incorporating his proposal in #83 (comment).