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