<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, May 19, 2016 at 10:07 AM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">So... what did we decide for arrays of atomic counters? Do we need an extra pass for that or ... ? Also... how does this handle the possibly unsized (actually draw-time-sized) array at the end of an SSBO? </blockquote><div> </div><div>This patch only matters for the block index not the offset into the buffer. For atomics, the "block index" isn't allowed to be indirected (Maybe they don't even have one?). For UBOs and SSBOs, we set the correct buffer size in the surface state so the hardware won't let it go outside. This should also work for SSBO unsized arrays. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> For UBOs, I think this patch is definitely sufficient, and I think it improves things quite a lot for SSBOs. We may need some more, but this patch is Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> <div class="HOEnZb"><div class="h5"> On 05/19/2016 12:21 AM, Jason Ekstrand wrote: > This prevents array overflow when the block is actually an array of UBOs or > SSBOs. On some hardware such as i965, such overflows can cause GPU hangs. > --- > src/compiler/glsl/ir_optimization.h | 2 +- > src/compiler/glsl/linker.cpp | 3 ++- > src/compiler/glsl/lower_ubo_reference.cpp | 36 +++++++++++++++++++++++++++---- > src/mesa/drivers/dri/i965/brw_compiler.c | 1 + > src/mesa/main/mtypes.h | 3 +++ > 5 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h > index 5fc2740..4afa37e 100644 > --- a/src/compiler/glsl/ir_optimization.h > +++ b/src/compiler/glsl/ir_optimization.h > @@ -123,7 +123,7 @@ bool lower_clip_distance(gl_shader *shader); > void lower_output_reads(unsigned stage, exec_list *instructions); > bool lower_packing_builtins(exec_list *instructions, int op_mask); > void lower_shared_reference(struct gl_shader *shader, unsigned *shared_size); > -void lower_ubo_reference(struct gl_shader *shader); > +void lower_ubo_reference(struct gl_shader *shader, bool clamp_block_indices); > void lower_packed_varyings(void *mem_ctx, > unsigned locations_used, ir_variable_mode mode, > unsigned gs_input_vertices, gl_shader *shader, > diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp > index 71a71df..07c8263 100644 > --- a/src/compiler/glsl/linker.cpp > +++ b/src/compiler/glsl/linker.cpp > @@ -4879,7 +4879,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) > &ctx->Const.ShaderCompilerOptions[i]; > > if (options->LowerBufferInterfaceBlocks) > - lower_ubo_reference(prog->_LinkedShaders[i]); > + lower_ubo_reference(prog->_LinkedShaders[i], > + options->ClampBlockIndicesToArrayBounds); > > if (options->LowerShaderSharedVariables) > lower_shared_reference(prog->_LinkedShaders[i], > diff --git a/src/compiler/glsl/lower_ubo_reference.cpp b/src/compiler/glsl/lower_ubo_reference.cpp > index 1a0140f..749deed 100644 > --- a/src/compiler/glsl/lower_ubo_reference.cpp > +++ b/src/compiler/glsl/lower_ubo_reference.cpp > @@ -44,8 +44,10 @@ namespace { > class lower_ubo_reference_visitor : > public lower_buffer_access::lower_buffer_access { > public: > - lower_ubo_reference_visitor(struct gl_shader *shader) > - : shader(shader), struct_field(NULL), variable(NULL) > + lower_ubo_reference_visitor(struct gl_shader *shader, > + bool clamp_block_indices) > + : shader(shader), clamp_block_indices(clamp_block_indices), > + struct_field(NULL), variable(NULL) > { > } > > @@ -104,6 +106,7 @@ public: > ir_visitor_status visit_enter(ir_call *ir); > > struct gl_shader *shader; > + bool clamp_block_indices; > struct gl_uniform_buffer_variable *ubo_var; > const struct glsl_struct_field *struct_field; > ir_variable *variable; > @@ -242,6 +245,26 @@ interface_field_name(void *mem_ctx, char *base_name, ir_rvalue *d, > return NULL; > } > > +static ir_rvalue * > +clamp_to_array_bounds(void *mem_ctx, ir_rvalue *index, const glsl_type *type) > +{ > + assert(type->is_array()); > + > + const unsigned array_size = type->arrays_of_arrays_size(); > + > + ir_constant *max_index = new(mem_ctx) ir_constant(array_size - 1); > + max_index->type = index->type; > + > + ir_constant *zero = new(mem_ctx) ir_constant(0); > + zero->type = index->type; > + > + if (index->type->base_type == GLSL_TYPE_INT) > + index = max2(index, zero); > + index = min2(index, max_index); > + > + return index; > +} > + > void > lower_ubo_reference_visitor::setup_for_load_or_store(void *mem_ctx, > ir_variable *var, > @@ -258,6 +281,11 @@ lower_ubo_reference_visitor::setup_for_load_or_store(void *mem_ctx, > interface_field_name(mem_ctx, (char *) var->get_interface_type()->name, > deref, &nonconst_block_index); > > + if (nonconst_block_index && clamp_block_indices) { > + nonconst_block_index = > + clamp_to_array_bounds(mem_ctx, nonconst_block_index, var->type); > + } > + > /* Locate the block by interface name */ > unsigned num_blocks; > struct gl_uniform_block **blocks; > @@ -1062,9 +1090,9 @@ lower_ubo_reference_visitor::visit_enter(ir_call *ir) > } /* unnamed namespace */ > > void > -lower_ubo_reference(struct gl_shader *shader) > +lower_ubo_reference(struct gl_shader *shader, bool clamp_block_indices) > { > - lower_ubo_reference_visitor v(shader); > + lower_ubo_reference_visitor v(shader, clamp_block_indices); > > /* Loop over the instructions lowering references, because we take > * a deref of a UBO array using a UBO dereference as the index will > diff --git a/src/mesa/drivers/dri/i965/brw_compiler.c b/src/mesa/drivers/dri/i965/brw_compiler.c > index 82131db..3f17589 100644 > --- a/src/mesa/drivers/dri/i965/brw_compiler.c > +++ b/src/mesa/drivers/dri/i965/brw_compiler.c > @@ -188,6 +188,7 @@ brw_compiler_create(void *mem_ctx, const struct brw_device_info *devinfo) > } > > compiler->glsl_compiler_options[i].LowerBufferInterfaceBlocks = true; > + compiler->glsl_compiler_options[i].ClampBlockIndicesToArrayBounds = true; > } > > compiler->glsl_compiler_options[MESA_SHADER_TESS_CTRL].EmitNoIndirectInput = false; > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > index 569e0ac..24f442f 100644 > --- a/src/mesa/main/mtypes.h > +++ b/src/mesa/main/mtypes.h > @@ -2949,6 +2949,9 @@ struct gl_shader_compiler_options > > GLboolean LowerBufferInterfaceBlocks; /**< Lower UBO and SSBO access to intrinsics. */ > > + /** Clamp UBO and SSBO block indices so they don't go out-of-bounds. */ > + GLboolean ClampBlockIndicesToArrayBounds; > + > GLboolean LowerShaderSharedVariables; /**< Lower compute shader shared > * variable access to intrinsics. */ > > </div></div></blockquote></div> </div></div>