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

[Iago Toral]

On Tue, 2015-06-23 at 15:45 -0700, Jordan Justen wrote:
> 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."

Sure, that makes things more clear.

> 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.

Thanks Jordan!

Iago

> > > 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
> > > 
> > 
> > 
>