[Mesa-dev] [PATCH v2 4/4] anv/pipeline: Bounds-check resource indices when robuts_buffer_access is enabled

[Jason Ekstrand]

On May 19, 2016 12:50 AM, "Michael Schellenberger Costa" <
mschellenbergercosta at googlemail.com> wrote:
>
> Hi Jason,
>
> Am 19.05.2016 um 09:22 schrieb Jason Ekstrand:
> > ---
> >  src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 52
++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > index 91f4322..7e66149 100644
> > --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > @@ -29,6 +29,9 @@ struct apply_pipeline_layout_state {
> >     nir_shader *shader;
> >     nir_builder builder;
> >
> > +   struct anv_pipeline_layout *layout;
> > +   bool add_bounds_checks;
> > +
> >     struct {
> >        BITSET_WORD *used;
> >        uint8_t *surface_offsets;
> > @@ -110,17 +113,15 @@ lower_res_index_intrinsic(nir_intrinsic_instr
*intrin,
> >     uint32_t binding = nir_intrinsic_binding(intrin);
> >
> >     uint32_t surface_index = state->set[set].surface_offsets[binding];
> > +   uint32_t array_size =
> > +      state->layout->set[set].layout->binding[binding].array_size;
> >
> > -   nir_const_value *const_block_idx =
> > -      nir_src_as_const_value(intrin->src[0]);
> > +   nir_ssa_def *block_index = nir_ssa_for_src(b, intrin->src[0], 1);
> >
> > -   nir_ssa_def *block_index;
> > -   if (const_block_idx) {
> > -      block_index = nir_imm_int(b, surface_index +
const_block_idx->u32[0]);
> > -   } else {
> > -      block_index = nir_iadd(b, nir_imm_int(b, surface_index),
> > -                             nir_ssa_for_src(b, intrin->src[0], 1));
> > -   }
> > +   if (state->add_bounds_checks)
> > +      block_index = nir_umax(b, block_index, nir_imm_int(b, array_size
- 1));
> > +
> > +   block_index = nir_iadd(b, nir_imm_int(b, surface_index),
block_index);
> Here you do
> |       if (state->add_bounds_checks)
> |           block_index = nir_umax(...);
> |       block_index = nir_iadd(...);
>
> Below you do
> |       nir_ssa_def *index = iadd(...);
> |       if (state->add_bounds_checks)
> |           index = nir_umax(...);
>
> Are both functionally equivalent? Also why do you one time do

No but both are correct.  In the case above, we are computing

resource_index(set, binding, index) = resource_index(set, binding, 0) +
umin(index, bound)

(I just realized I got umin and umax backwards).  In the second case, the
index is split into a base and an indirect so we actually have 3 pieces:
resource_index(set, binding, 0), base_offset, and the indirect.  This means
that instead of the index above we have base+indirect.  Does that make
sense?

> | nir_ssa_def *block_index = nir_ssa_for_src()
>
> and the other time put that directly into nir_iadd()?
>
> Could you unify the code so that both cases are equivalent?
> --Michael
>
>
> >
> >     assert(intrin->dest.is_ssa);
> >     nir_ssa_def_rewrite_uses(&intrin->dest.ssa,
nir_src_for_ssa(block_index));
> > @@ -129,16 +130,24 @@ lower_res_index_intrinsic(nir_intrinsic_instr
*intrin,
> >
> >  static void
> >  lower_tex_deref(nir_tex_instr *tex, nir_deref_var *deref,
> > -                unsigned *const_index, nir_tex_src_type src_type,
> > +                unsigned *const_index, unsigned array_size,
> > +                nir_tex_src_type src_type,
> >                  struct apply_pipeline_layout_state *state)
> >  {
> > +   nir_builder *b = &state->builder;
> > +
> >     if (deref->deref.child) {
> >        assert(deref->deref.child->deref_type == nir_deref_type_array);
> >        nir_deref_array *deref_array =
nir_deref_as_array(deref->deref.child);
> >
> > -      *const_index += deref_array->base_offset;
> > -
> >        if (deref_array->deref_array_type ==
nir_deref_array_type_indirect) {
> > +         nir_ssa_def *index =
> > +            nir_iadd(b, nir_imm_int(b, deref_array->base_offset),
> > +                        nir_ssa_for_src(b, deref_array->indirect, 1));
> > +
> > +         if (state->add_bounds_checks)
> > +            index = nir_umax(b, index, nir_imm_int(b, array_size - 1));
> > +
> >           nir_tex_src *new_srcs = rzalloc_array(tex, nir_tex_src,
> >                                                 tex->num_srcs + 1);
> >
> > @@ -154,10 +163,11 @@ lower_tex_deref(nir_tex_instr *tex, nir_deref_var
*deref,
> >            * first-class texture source.
> >            */
> >           tex->src[tex->num_srcs].src_type = src_type;
> > +         nir_instr_rewrite_src(&tex->instr, &tex->src[tex->num_srcs],
> > +                               nir_src_for_ssa(index));
> >           tex->num_srcs++;
> > -         assert(deref_array->indirect.is_ssa);
> > -         nir_instr_rewrite_src(&tex->instr, &tex->src[tex->num_srcs -
1].src,
> > -                               deref_array->indirect);
> > +      } else {
> > +         *const_index += MIN2(deref_array->base_offset, array_size -
1);
> >        }
> >     }
> >  }
> > @@ -182,17 +192,23 @@ lower_tex(nir_tex_instr *tex, struct
apply_pipeline_layout_state *state)
> >     /* No one should have come by and lowered it already */
> >     assert(tex->texture);
> >
> > +   state->builder.cursor = nir_before_instr(&tex->instr);
> > +
> >     unsigned set = tex->texture->var->data.descriptor_set;
> >     unsigned binding = tex->texture->var->data.binding;
> > +   unsigned array_size =
> > +      state->layout->set[set].layout->binding[binding].array_size;
> >     tex->texture_index = state->set[set].surface_offsets[binding];
> > -   lower_tex_deref(tex, tex->texture, &tex->texture_index,
> > +   lower_tex_deref(tex, tex->texture, &tex->texture_index, array_size,
> >                     nir_tex_src_texture_offset, state);
> >
> >     if (tex->sampler) {
> >        unsigned set = tex->sampler->var->data.descriptor_set;
> >        unsigned binding = tex->sampler->var->data.binding;
> > +      unsigned array_size =
> > +         state->layout->set[set].layout->binding[binding].array_size;
> >        tex->sampler_index = state->set[set].sampler_offsets[binding];
> > -      lower_tex_deref(tex, tex->sampler, &tex->sampler_index,
> > +      lower_tex_deref(tex, tex->sampler, &tex->sampler_index,
array_size,
> >                        nir_tex_src_sampler_offset, state);
> >     }
> >
> > @@ -254,6 +270,8 @@ anv_nir_apply_pipeline_layout(struct anv_pipeline
*pipeline,
> >
> >     struct apply_pipeline_layout_state state = {
> >        .shader = shader,
> > +      .layout = layout,
> > +      .add_bounds_checks = pipeline->device->robust_buffer_access,
> >     };
> >
> >     void *mem_ctx = ralloc_context(NULL);
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160519/17657f77/attachment.html>