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. r-b? -Jordan > > > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> > > --- > > src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 3 ++- > > src/mesa/drivers/dri/i965/brw_shader.cpp | 14 ++++++++++++++ > > src/mesa/drivers/dri/i965/brw_shader.h | 6 ++++++ > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp > > b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp > > index c7628dc..3a28c8d 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp > > @@ -93,7 +93,8 @@ is_expression(const fs_visitor *v, const fs_inst *const > > inst) > > case SHADER_OPCODE_LOAD_PAYLOAD: > > return !inst->is_copy_payload(v->alloc); > > default: > > - return inst->is_send_from_grf() && !inst->has_side_effects(); > > + return inst->is_send_from_grf() && !inst->has_side_effects() && > > + !inst->is_volatile(); > > } > > } > > > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp > > b/src/mesa/drivers/dri/i965/brw_shader.cpp > > index 2324b56..be911ed 100644 > > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp > > @@ -969,6 +969,20 @@ backend_instruction::has_side_effects() const > > } > > } > > > > +bool > > +backend_instruction::is_volatile() const > > +{ > > + switch (opcode) { > > + case SHADER_OPCODE_UNTYPED_SURFACE_READ: > > + case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL: > > + case SHADER_OPCODE_TYPED_SURFACE_READ: > > + case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > #ifndef NDEBUG > > static bool > > inst_is_in_block(const bblock_t *block, const backend_instruction *inst) > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.h > > b/src/mesa/drivers/dri/i965/brw_shader.h > > index b33b08f..35ee210 100644 > > --- a/src/mesa/drivers/dri/i965/brw_shader.h > > +++ b/src/mesa/drivers/dri/i965/brw_shader.h > > @@ -115,6 +115,12 @@ struct backend_instruction : public exec_node { > > * optimize these out unless you know what you are doing. > > */ > > bool has_side_effects() const; > > + > > + /** > > + * True if the instruction might be affected by side effects of other > > + * instructions. > > + */ > > + bool is_volatile() const; > > #else > > struct backend_instruction { > > struct exec_node link; > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev