On 2015-06-17 22:57:27, Iago Toral wrote:
> On Wed, 2015-06-17 at 17:20 -0700, Jordan Justen wrote:
> > I wanted to question whether this was required, based on this text
> > from the extension spec:
> >
> > "The ability to write to buffer objects creates the potential for
> > multiple independent shader invocations to read and write the same
> > underlying memory. The same issue exists with the
> > ARB_shader_image_load_store extension provided in OpenGL 4.2, which
> > can write to texture objects and buffers. In both cases, the
> > specification makes few guarantees related to the relative order of
> > memory reads and writes performed by the shader invocations."
> >
> > But I'm not sure if we can reconcile CSE with 'memoryBarrier' and
> > 'barrier'. curro, any thoughts from image load/store?
>
> I think the problem is within the same thread, that text above talks
> about multiple invocations reading from and writing to the same
> location, but within the same invocation, the order of reads and writes
> must be preserved:
>
> "Buffer variable memory reads and writes within a single shader
> invocation are processed in order. However, the order of reads and
> writes performed in one invocation relative to those performed by
> another invocation is largely undefined."
>
> For example, if X is a shader storage buffer variable and we have code
> like this with just one invocation:
>
> ssbo_store(X, 1);
> a = ssbo_load(X) + 1 // a = 2
> ssbo_store(X, 2);
> b = ssbo_load(X) + 1; // b = 3
>
> CSE could mess it up like this:
>
> ssbo_store(X, 1);
> tmp = ssbo_load(X) + 1 // tmp = 2
> a = tmp;
> ssbo_store(X, 2);
> b = tmp;
>
> which would be incorrect. I think I wrote this patch after seeing
> something like this happening. The CSE pass clearly states that it does
> not support write variables after all.
>
> Also, notice the same would apply if there are multiple invocations but
> the shader code used something like gl_VertexID or gl_FragCoord to make
> each invocation read from/write to a different address within the SSBO
> buffer (I imagine this is the usual way to operate with SSBOs). In these
> cases, even if we have multiple invocations, keeping the relative order
> of reads and writes within each one is necessary.
Ok. Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> >
> > On 2015-06-03 00:01:13, Iago Toral Quiroga wrote:
> > > SSBOs are read/write and this CSE pass only handles read-only variables.
> > > ---
> > > src/glsl/opt_cse.cpp | 33 ++++++++++++++++++++++++++++++++-
> > > 1 file changed, 32 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/glsl/opt_cse.cpp b/src/glsl/opt_cse.cpp
> > > index 4b8e9a0..a05ab46 100644
> > > --- a/src/glsl/opt_cse.cpp
> > > +++ b/src/glsl/opt_cse.cpp
> > > @@ -245,6 +245,28 @@ contains_rvalue(ir_rvalue *haystack, ir_rvalue
> > > *needle)
> > > }
> > >
> > > static bool
> > > +expression_contains_ssbo_load(ir_expression *expr)
> > > +{
> > > + if (expr->operation == ir_binop_ssbo_load)
> > > + return true;
> > > +
> > > + for (unsigned i = 0; i < expr->get_num_operands(); i++) {
> > > + ir_rvalue *op = expr->operands[i];
> > > + if (op->ir_type == ir_type_expression &&
> > > + expression_contains_ssbo_load(op->as_expression())) {
> > > + return true;
> > > + } else if (op->ir_type == ir_type_swizzle) {
> > > + ir_swizzle *swizzle = op->as_swizzle();
> > > + ir_expression *val = swizzle->val->as_expression();
> > > + if (val && expression_contains_ssbo_load(val))
> > > + return true;
> > > + }
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > +static bool
> > > is_cse_candidate(ir_rvalue *ir)
> > > {
> > > /* Our temporary variable assignment generation isn't ready to handle
> > > @@ -260,7 +282,16 @@ is_cse_candidate(ir_rvalue *ir)
> > > * to variable-index array dereferences at some point.
> > > */
> > > switch (ir->ir_type) {
> > > - case ir_type_expression:
> > > + case ir_type_expression: {
> > > + /* Skip expressions involving SSBO loads, since these operate on
> > > + * read-write variables, meaning that the same ssbo_load
> > > expression
> > > + * may return a different value if the underlying buffer storage
> > > + * is written in between.
> > > + */
> > > + if (expression_contains_ssbo_load(ir->as_expression()))
> > > + return false;
> > > + }
> > > + break;
> > > case ir_type_texture:
> > > break;
> > > default:
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > 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
