[Mesa-dev] [PATCH] i965/fs: Disable CSE optimization for untyped & typed surface reads

[Jordan Justen]

On 2015-10-22 00:06:37, Iago Toral wrote:
> On Wed, 2015-10-21 at 23:24 -0700, Jordan Justen wrote:
> > On 2015-10-20 00:43:13, Iago Toral wrote:
> > > On Tue, 2015-10-20 at 00:12 -0700, Jordan Justen wrote:
> > > > An untyped surface read is volatile because it might be affected by a
> > > > write.
> > > > 
> > > > In the ES31-CTS.compute_shader.resources-max test, two back to back
> > > > read/modify/writes of an SSBO variable looked something like this:
> > > > 
> > > >   r1 = untyped_surface_read(ssbo_float)
> > > >   r2 = r1 + 1
> > > >   untyped_surface_write(ssbo_float, r2)
> > > >   r3 = untyped_surface_read(ssbo_float)
> > > >   r4 = r3 + 1
> > > >   untyped_surface_write(ssbo_float, r4)
> > > > 
> > > > And after CSE, we had:
> > > > 
> > > >   r1 = untyped_surface_read(ssbo_float)
> > > >   r2 = r1 + 1
> > > >   untyped_surface_write(ssbo_float, r2)
> > > >   r4 = r1 + 1
> > > >   untyped_surface_write(ssbo_float, r4)
> > > 
> > > Yeah, we cannot do CSE with SSBO loads. Patch looks good to me, but we
> > > should do the same in the vec4 CSE pass.
> > 
> > Yeah, I checked vec4 CSE. It looks like is_expression will
> > unconditionally return false for those opcodes.
> 
> Oh right.
> 
> > r-b?
> 
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> 
> FWIW, my ssbo load optimization pass is trying to "undo" this since it
> is all about doing CSE for ssbo loads that are safe to CSE, that is,
> when we know that we don't have stores/atomics that write to the same
> offset or memory barriers in between. I am trying to implement that in
> NIR though, so we still need this, to prevent i965 from trying to CSE
> the remaining loads it sees, since thise would not be safe to CSE.
> 
> Also, as I mentioned in another e-mail, we did not notice this issue
> earlier was because there are a couple of problems in i965 that make it
> quite difficult that the CSE pass identifies identical SSBO loads at the
> moment, but that is bound to change as soon as those things get
> eventually fixed.

Yeah. I only saw this after applying some optimization patches from krh.

-Jordan