Iago Toral <itoral at igalia.com> writes:
> On Wed, 2015-08-05 at 22:22 +1000, Timothy Arceri wrote:
>> On Wed, 2015-08-05 at 13:45 +0200, Iago Toral wrote:
>> > On Wed, 2015-08-05 at 20:04 +1000, Timothy Arceri wrote:
>> > > On Wed, 2015-08-05 at 10:30 +0200, Iago Toral Quiroga wrote:
>> > > > ---
>> > > > src/glsl/ast_to_hir.cpp | 9 ++++++++-
>> > > > 1 file changed, 8 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> > > > index e834a46..518612d 100644
>> > > > --- a/src/glsl/ast_to_hir.cpp
>> > > > +++ b/src/glsl/ast_to_hir.cpp
>> > > > @@ -811,8 +811,15 @@ do_assignment(exec_list *instructions, struct
>> > > > _mesa_glsl_parse_state *state,
>> > > > }
>> > > >
>> > > > ir_variable *lhs_var = lhs->variable_referenced();
>> > > > - if (lhs_var)
>> > > > + if (lhs_var) {
>> > > > + if (lhs_var->data.image_read_only) {
>> > >
>> > > It looks like data.read_only is always set to true for images so wouldn't
>> > > this
>> > > already be caught already by the existing read-only check?
>> > >
>> > > else if (lhs_var != NULL && lhs_var->data.read_only) {
>> > > _mesa_glsl_error(&lhs_loc, state,
>> > > "assignment to read-only variable '%s'",
>> > > lhs_var->name);
>> >
>> > Not as it is now, because with SSBOs we only set image_read_only and not
>> > read_only when the readonly qualifier is used. I suppose this is what we
>> > are expected to do since the SSBO spec says that behavior for these
>> > qualifiers on SSBOs is the same as for images:
>> > https://www.opengl.org/registry/specs/ARB/shader_storage_buffer_object.txt
>> >
>> > "Modify Section 4.10, Memory Qualifiers (p. 71)"
>> > (...)
>> > "(insert after third paragraph, p. 73) 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. It
>> > is an error to assign to a buffer variable qualified with "readonly" or to
>> > read from a buffer variable qualified with "writeonly".
>> >
>> > What is a bit confusing for me is that images seem to set
>> > image_read_only depending on whether we used the readonly qualifier or
>> > not (like ssbos) but then they also set read_only to true
>> > unconditionally, so I guess there is a difference between both fields,
>>
>> Asking what the difference is was originally going to be my first question to
>> you :)
>>
>> > but I don't know what it is exactly, specially since you can also use
>> > writeonly on images, for example.
>>
>> So I really dont know much about images but after some reading the conclusion
>> I've come to is the qualifiers (image_read_only) are meant to limit how you
>> can use imageStore(), imageLoad() and imageAtomic*() etc.
>
> Looking at ARB_shader_image_load_store that seems consistent... In that
> case I imagine that we could just set read_only for buffer variables
> with the readonly qualifier instead of image_read_only and drop this
> patch. We will need to add, at least, write_only to ir_variable as well
> I guess... I imagine that the 3 other fields (image_coherent,
> image_restrict, image_volatile) do not have image-specific semantics
> like image_read_only and image_write_oly and can be shared with ssbos
> we do not have to replicate them in ir_variable as well (in that case we
> might want to rename them so it is clear that image_read_only and
> image_write_only really are special and specific to images)
>
> Curro, what do you think?
>
Yes, the image_* fields (which we should definitely rename now that they
also affect SSBO variables) are just the memory qualifiers defined by
the GLSL spec, in the case of images the difference between these and
the original ir_variable::read_only is clear: The former apply to the
memory pointed to by the variable, while the latter refers to the
variable itself -- So in principle one could have a fixed image uniform
(fixed in the sense of always pointing to the same image unit) pointing
to some write-only image (i.e. only ir_variable::read_only set), or a
image uniform mutated during the execution of the program pointing to a
read-only image (i.e. only ir_variable::image_read_only set -- Although
in practice this latter possibility is disallowed by the spec, which is
why images currently always have ir_variable::read_only set).
For buffer variables the distinction is blurred because syntactically
the variable is both pointer-to-storage and storage at the same time, so
AFAICT the approach you've taken of checking whether image_read_only is
set for the LHS of an assignment seems reasonable to me, because this
way all image_* memory qualifiers apply to the storage of buffer
variables consistently. But please let's make the check conditional on
the LHS being a buffer variable because this confusion between variable
and pointed-to-storage is SSBO-specific.
>> On the other hand read_only is the usual uniform restriction stoping you from
>> assigning to the variable directly e.g myImage = 1; which is why its always
>> set to true.
>>
>> If I'm correct I dont think this patch is needed.
>>
>> >
>> > In any case, since we have both read_only and image_read_only in
>> > ir_variable at present, I think it makes sense to have checks for both
>> > of them, if one of them ends up being redundant the right thing to do
>> > would be to kill it completely I guess, otherwise it only gets (even)
>> > more confusing.
>> >
>> > Iago
>> >
>> > >
>> > > > + _mesa_glsl_error(&lhs_loc, state,
>> > > > + "assignment to read-only variable `%s'",
>> > > > + lhs_var->name);
>> > > > + error_emitted = true;
>> > > > + }
>> > > > lhs_var->data.assigned = true;
>> > > > + }
>> > > >
>> > > > if (!error_emitted) {
>> > > > if (non_lvalue_description != NULL) {
>> > >
>> >
>> >
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150808/1d433be7/attachment.sig>