[Mesa-dev] [PATCH v3 13/16] glsl: Add an option to clamp block indices when lowering UBO/SSBOs
Timothy Arceri
timothy.arceri at collabora.com
Mon May 23 23:59:08 UTC 2016
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 <ian.d.romanick at intel.com>
> > ---
> > 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 <kenneth at whitecape.org>
>
> >
> > +
> > + 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
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list