[Mesa-dev] [PATCH 11/13] glsl: Add an option to clamp block indices when lowering UBO/SSBOs
Jason Ekstrand
jason at jlekstrand.net
Thu May 19 21:42:43 UTC 2016
On Thu, May 19, 2016 at 10:07 AM, Ian Romanick <idr at freedesktop.org> wrote:
> 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?
>
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.
> 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 <ian.d.romanick at intel.com>
>
> 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. */
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160519/e2eb670f/attachment.html>
More information about the mesa-dev
mailing list