[Mesa-dev] [PATCH v2 26/82] glsl: Don't do copy propagation on buffer variables

[Jordan Justen]

On 2015-06-22 23:38:14, Iago Toral wrote:
> On Mon, 2015-06-22 at 14:28 -0700, Jordan Justen wrote:
> > 24-26 once again makes me wonder if these optimization *can* be used
> > with SSBOs based on the same ext spec wording I referenced before:
> > 
> > "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."
> > 
> > In these patches "other threads" were specifically mentioned.
> > 
> > Did these patches also prevent bad things from happening in generated
> > code? (Like mentioned for patch 23.)
> 
> I think the problem here is the possibility for shaders to use
> memoryBarrier():
> 
> "SHADER_STORAGE_BARRIER_BIT:  Memory accesses using shader buffer
> variables issued after the barrier will reflect data written by
> shaders prior to the barrier.  Additionally, assignments to and atomic
> operations performed on shader buffer variables after the barrier will
> not execute until all memory accesses (e.g., loads, stores, texture
> fetches, vertex fetches) initiated prior to the barrier complete."
> 
> I think in these cases we can't allow these optimizations to kick in. 
> 
> That said, maybe we can check if we are using any memorybarriers in the
> shader code and decide if we want to enable these optimizations for
> ssbos based on that. I think we can try to do that in a later patch.

Ok. What do you think about updating the commit messages on these
three patches?

For example, currently you have:

"Since the backing storage for these is shared we cannot ensure that
 the value won't change by writes from other threads."

How does something like this sound?

"Since the backing storage for these is shared we cannot ensure that
 the value won't change by writes from other threads. Normally SSBO
 accesses are not guaranteed to be syncronized with other threads,
 except when memoryBarrier is used. So, we might be able to optimize
 some SSBO accesses, but for now we always take the safe path and emit
 the SSBO access."

With a change like that to these commit messages, you can add
Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
to all 3 patches.

> > On 2015-06-03 00:01:16, Iago Toral Quiroga wrote:
> > > Since the backing storage for these is shared we cannot ensure that the
> > > value won't change by writes from other threads.
> > > ---
> > >  src/glsl/opt_copy_propagation.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/glsl/opt_copy_propagation.cpp b/src/glsl/opt_copy_propagation.cpp
> > > index 806027b..f206995 100644
> > > --- a/src/glsl/opt_copy_propagation.cpp
> > > +++ b/src/glsl/opt_copy_propagation.cpp
> > > @@ -330,7 +330,7 @@ ir_copy_propagation_visitor::add_copy(ir_assignment *ir)
> > >           */
> > >          ir->condition = new(ralloc_parent(ir)) ir_constant(false);
> > >          this->progress = true;
> > > -      } else {
> > > +      } else if (lhs_var->data.mode != ir_var_shader_storage) {
> > >          entry = new(this->acp) acp_entry(lhs_var, rhs_var);
> > >          this->acp->push_tail(entry);
> > >        }
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
>