On Mon, 2016-05-23 at 16:07 -0700, Kenneth Graunke wrote: > On Friday, May 20, 2016 4:53:24 PM PDT 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. > > > > Reviewed-by: Ian Romanick <[email protected]> > > --- > > 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 c7a7c63..446b1fc 100644 > > --- a/src/compiler/glsl/linker.cpp > > +++ b/src/compiler/glsl/linker.cpp > > @@ -4882,7 +4882,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(); > Can you actually have arrays of arrays of UBOs/SSBOs?
Yes. Although Mesa is the only implementation to actually support it last time I checked. > Even still, isn't this an index into the outermost array? So don't > we > want to simply clamp it to type->length, rather than the flattened > multi-dimensional size? > > With that fixed or refuted, the series is: > Reviewed-by: Kenneth Graunke <[email protected]> > > > > > + > > + 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. */ > > > > > > > _______________________________________________ > mesa-dev mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
