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. > > 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.
I meant to include a quote from the ARB_shader_image_load_store spec: Memory accesses to image variables declared using the "readonly" qualifier may only read the underlying memory, which is treated as read-only memory and cannot be written to. It is an error to pass an image variable qualified with "readonly" to imageStore() or other built-in functions that modify image memory. Memory accesses to image variables declared using the "writeonly" qualifier may only write the underlying memory; the underlying memory cannot be read. It is an error to pass an image variable qualified with "writeonly" to imageLoad() or other built-in functions that read image memory. > > > > > 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) { > > > > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev