[Mesa-dev] [PATCH v2 23/82] glsl: Do not do CSE for expressions involving SSBO loads

[Jordan Justen]

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.justen at 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 at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
>