Description of UniformConstant storage class in spec

[mmoult]

This is specifically in regards to the online specification. Please redirect me if this is the wrong place to file a ticket for that.

I think the description for the UniformConstant storage class in the unified specification is partially incorrect, and could be improved. It reads:

Shared externally, visible across all invocations. Graphics uniform memory. OpenCL constant memory. Variables declared with this storage class are read-only. They may have initializers, as allowed by the client API.

(italic emphasis mine)

Not all graphics uniform memory variables are strictly-speaking read-only. Consider this short shader in GLSL:

#version 460

layout(set = 0, binding = 1, rgba8) uniform writeonly image2D image;

void main()
{
    imageStore(image, ivec2(0, 0), vec4(1.0));
}

image is a graphics uniform memory variable, but its memory is modified by the imageStore. Continuing, we use glslang to convert the shader to SPIR-V. We get:

%image = OpVariable %_ptr_UniformConstant_7 UniformConstant

Assuming that glslang's translation is correct, we have here a variable of storage class UniformConstant with necessarily mutable data. Granted, it would be illegal to reassign the image (and perhaps that is meant by "constant"), but the confusion persists. In case it isn't clear, this is the same concept as C's const type* ptr versus type* const ptr.

I would suggest a change of wording:

"Variables declared with this storage class are read-only may not be reassigned."

To my knowledge, that correction is accurate, but I am happy to brainstorm alternatives if my statement is proven wrong.

Alternatively, we could decide that UniformConstants should in fact, be read-only. If so, the implication that "Graphics uniform memory" falls under UniformConstant is incorrect. Admittedly, I am not sure how we should change the spec in that case. Maybe something like:

"Graphics non-image uniform memory."

If we continue down this route, I can create an issue for glslang to change its translation of GLSL uniforms.

[alan-baker]

Glslang's translation is correct. The object in UniformConstant is a handle and not the image itself. This is non-obvious. You cannot store or reassign the image handle, but you can modify the contents of the image. The easiest way to view this difference is by examing OpImageTexelPointer which actually returns a pointer into the Image storage class. That is where the actual texels are. Similarly it is why you can decorate a storage image with NonReadable and still perform queries on the image object (because only handle metadata is accessed).

[mmoult]

Thank you for your response. I agree that the idea of the Image being a handle and not the image itself is non-obvious. Ideally, this would be clarified in the specification.

Let's consider this idea further:

As far as I am aware, all OpVariables are effectively handles/pointers to the data they contain. SPIR-V follows SSA, so of course we cannot reassign the object directly, but we get around this with loads and stores. Just to make sure I understand what you are saying- this is not the handle you are talking about, right? In other words, if we see:

%7 = OpTypeImage %float 2D 0 0 0 2 Rgba8

it is really an image handle type being declared, right? And since the type of the variable is an OpTypePointer, we could accurately say by your idea that %image is a pointer to an image pointer. Perhaps that is what is intended (I recognize you have much more experience with SPIR-V than I do), but I don't understand why we would want that extra level of indirection. Let's consider a case which poses some complexity:

%10 = OpLoad %7 %image
%11 = OpLoad %7 %image
OpImageWrite %10 %20 %30
OpImageWrite %11 %20 %31
...
%37 = OpImageRead %v4float %10 %20

What is the desired value of %37 after executing these lines? By the image handle idea, I think %37 == %31, but now we have introduced a case where a non-pointer data (or at least, not explicitly a pointer by the spec) has been changed by an instruction which doesn't even reference it. I think this is undesirable and it conflicts with the pattern established by OpLoads for other types.

You said that I can decorate a storage image with NonReadable and still perform queries on the image object. Would you please provide an example? I tried using:

#version 460

layout(location = 1) in uvec3 coords;
layout(set = 0, binding = 1, rgba8) uniform writeonly image2D image;

void main()
{
    vec4 got = imageLoad(image, ivec2(coords.xy));
}

but glslang rejected it until I removed the writeonly decoration. Perhaps I need to update the glslang version I have been using, but I thought 11:14.3.0 was fairly current.

Please explain a little more what you meant with your OpImageTexelPointer example. It is also possible to create pointers to specific entries in aggregates, so I don't see how that instruction is markedly different.

Lastly, is there a way to designate that an image (not the handle) is read-only or write-only? If we accept the idea that OpImage is just a handle to an image, then I don't know how we would go about describing the read/write properties of the image itself. It seems easier to me to reject the image handle idea- but please let me know if I am missing something here.

[alan-baker]

Thank you for your response. I agree that the idea of the Image being a handle and not the image itself is non-obvious. Ideally, this would be clarified in the specification.

Let's consider this idea further:

As far as I am aware, all OpVariables are effectively handles/pointers to the data they contain. SPIR-V follows SSA, so of course we cannot reassign the object directly, but we get around this with loads and stores. Just to make sure I understand what you are saying- this is not the handle you are talking about, right? In other words, if we see:

%7 = OpTypeImage %float 2D 0 0 0 2 Rgba8

it is really an image handle type being declared, right? And since the type of the variable is an OpTypePointer, we could accurately say by your idea that %image is a pointer to an image pointer. Perhaps that is what is intended (I recognize you have much more experience with SPIR-V than I do), but I don't understand why we would want that extra level of indirection.

The idea behind OpVariable have a pointer result type is that it is the address of the data. An object of OpTypeImage is a handle to an image. You could view as a pointer and some metadata. So a more accurate C/C++ style of declaration would be:

struct ImageType {
  uint32_t width;
  // other metadata
  TexelDataType texels[width]...; // depending on dimensionality
}

SPIR-V says the ImageType structure is not mutable, but the texel data may be mutable (e.g. for storage images).

Let's consider a case which poses some complexity:

%10 = OpLoad %7 %image
%11 = OpLoad %7 %image
OpImageWrite %10 %20 %30
OpImageWrite %11 %20 %31
...
%37 = OpImageRead %v4float %10 %20

What is the desired value of %37 after executing these lines? By the image handle idea, I think %37 == %31, but now we have introduced a case where a non-pointer data (or at least, not explicitly a pointer by the spec) has been changed by an instruction which doesn't even reference it. I think this is undesirable and it conflicts with the pattern established by OpLoads for other types.

The desired value (assuming you haven't created a data race) is the same as %31. Program order within a single invocation should prevent reordering those writes and they both access the same underlying memory.

You said that I can decorate a storage image with NonReadable and still perform queries on the image object. Would you please provide an example? I tried using:

#version 460

layout(location = 1) in uvec3 coords;
layout(set = 0, binding = 1, rgba8) uniform writeonly image2D image;

void main()
{
    vec4 got = imageLoad(image, ivec2(coords.xy));
}

but glslang rejected it until I removed the writeonly decoration. Perhaps I need to update the glslang version I have been using, but I thought 11:14.3.0 was fairly current.

The queries I was referring to were things like getting the image dimensions or number mip levels (OpImageQuery instructions). Those only access the metadata so can be done on a NonReadable image. Your example is accessing the texel data.

Please explain a little more what you meant with your OpImageTexelPointer example. It is also possible to create pointers to specific entries in aggregates, so I don't see how that instruction is markedly different.

OpImageTexelPointer is like an OpAccessChain into a image. It uses image coordinates to specify a particular texel. That instruction is used only with atomic instructions.

Lastly, is there a way to designate that an image (not the handle) is read-only or write-only? If we accept the idea that OpImage is just a handle to an image, then I don't know how we would go about describing the read/write properties of the image itself. It seems easier to me to reject the image handle idea- but please let me know if I am missing something here.

Different APIs do this differently. In OpenCL, you can use the access qualifier image operand. In Vulkan, you'd use the NonReadable or NonWritable decorations on the image variable. However, note that the Sampled operand of the OpTypeImage imposes some restrictions on possible instructions that it can be used with (e.g. cannot write an image with Sampled == 1).

[mmoult]

To summarize, here are things I have gathered from our conversation but cannot find anywhere in the specification:

• "OpTypeImage is a struct containing width and other fields along with a pointer to texel data." -- This seems to enforce more specificity than what the spec currently says: "This type is opaque: values of this type have no defined physical size or bit pattern." I agree that the implementation of an image handle is probably most reasonable for supporting the OpImageQuery* instructions, but there are other ways to implement the behavior, so I am surprised at your certainty.
• The fields in OpTypeImage are all immutable (regardless of the instance's storage class)." -- You said that the spec says this, but I cannot find it anywhere. In fact, I cannot find the word "mutable" anywhere in the spec, so I am not sure what you are referring to. Maybe you meant that because my example used UniformConstant that the fields needed to be read-only?
• NonReadable and NonWritable decorations which apply to instances of OpTypeImage actually apply to the underlying texel data- not the handle struct (which as said in the previous point, has immutable fields regardless)." -- This point is nonobvious and warrants clarification in the spec.
• "Since the OpTypeImage does not own the texel data (merely holding a pointer to it), there can be several image instances which point to the same texel data. This means that changes to the texel data via one image instance may affect the texel data of another image instance." -- This contrasts sharply from what I have come to expect with other aggregates. Admittedly, I cannot find any clear evidence that OpLoad creates a deep copy (although I believe that to be true), but "Khronos SPIR-V Issue # 308" disallows the loading or copying of runtime arrays: I don't see why loading would be an issue if there wasn't a copy involved.

You say that "Program order within a single invocation should prevent reordering those writes and they both access the same underlying memory", but the point I was trying to make is that even if they do access the same memory, that fact is not readily apparent because the destination operands are different. This idea adds a constraint on any optimizer to recursively track the source for all image operands before reordering (something which I thought was unique to pointers).

I think the takeaway is that if the image handle idea is correct, then it has far too many correctness repercussions to leave undocumented. I am sure you are a busy person, can you point me in the right direction of where I could propose formal edits to the spec to include the information discussed?

[alan-baker]

To summarize, here are things I have gathered from our conversation but cannot find anywhere in the specification:

• "OpTypeImage is a struct containing width and other fields along with a pointer to texel data." -- This seems to enforce more specificity than what the spec currently says: "This type is opaque: values of this type have no defined physical size or bit pattern." I agree that the implementation of an image handle is probably most reasonable for supporting the OpImageQuery* instructions, but there are other ways to implement the behavior, so I am surprised at your certainty.

This is a mental model. It is sufficient to communicate that there is (often) more at work than just a pointer to data.

• The fields in OpTypeImage are all immutable (regardless of the instance's storage class)." -- You said that the spec says this, but I cannot find it anywhere. In fact, I cannot find the word "mutable" anywhere in the spec, so I am not sure what you are referring to. Maybe you meant that because my example used UniformConstant that the fields needed to be read-only?

This is just fundamental to all types in SPIR-V. If I declare OpTypeInt 32 1, I cannot change the signedness of that type. The mutable refers to the underlying data of the image (the texels), not the properties of the type (e.g. whether it is multisampled).

• NonReadable and NonWritable decorations which apply to instances of OpTypeImage actually apply to the underlying texel data- not the handle struct (which as said in the previous point, has immutable fields regardless)." -- This point is nonobvious and warrants clarification in the spec.

The decorations detail that they can be applied to storage images (that is an image with Sampled operand of 2). The query instructions indicate they do not read the actual texel data. So this information is a bit spread out.

• "Since the OpTypeImage does not own the texel data (merely holding a pointer to it), there can be several image instances which point to the same texel data. This means that changes to the texel data via one image instance may affect the texel data of another image instance." -- This contrasts sharply from what I have come to expect with other aggregates. Admittedly, I cannot find any clear evidence that OpLoad creates a deep copy (although I believe that to be true), but "Khronos SPIR-V Issue # 308" disallows the loading or copying of runtime arrays: I don't see why loading would be an issue if there wasn't a copy involved.

Images are not aggregates. The load does not load all the texels. This is why the idea of a handle is important and I agree this could probably be explained better somewhere (even if not directly in the spec).

Runtime array loading is disallowed for different reasons. Because the length is not known at compile time there would not likely be any efficient way to do this.

You say that "Program order within a single invocation should prevent reordering those writes and they both access the same underlying memory", but the point I was trying to make is that even if they do access the same memory, that fact is not readily apparent because the destination operands are different. This idea adds a constraint on any optimizer to recursively track the source for all image operands before reordering (something which I thought was unique to pointers).

That is a reasonable comment. It requires understanding the way images are modelled that I admit is non-obvious.

I think the takeaway is that if the image handle idea is correct, then it has far too many correctness repercussions to leave undocumented. I am sure you are a busy person, can you point me in the right direction of where I could propose formal edits to the spec to include the information discussed?

The SPIR WG will consider this feedback. Some of this information might be appropriate in the SPIR-V Guide (e.g. the proposed image chapter could be further fleshed out). Some is appropriate in the spec. We don't have a good way to take edits directly, but you could make suggestions here about which sections you'd think should be modified and we could take it under advisement.

[spencer-lunarg]

I plan to update the SPIRV-Guide new Image Access chapter

there is a Storage Class chapter with a small section on UniformConstant as you likely will not be to last person to get tripped up by this

As Alan said, there are things that can probably be updated in the spec, but the type of good GLSL examples you gave are something that don't really belong there and why we started the SPIRV-Guide to help spillover that type of information

[Naghasan]

This was discussed during last week's WG meeting, I have an action to update the specs based on your feedbacks (and thanks for providing them) and should be present in the next release of the specs.