[Mesa-dev] [PATCH v2 4/4] anv/pipeline: Bounds-check resource indices when robuts_buffer_access is enabled
---
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);
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_
Re: [Mesa-dev] [PATCH v2 4/4] anv/pipeline: Bounds-check resource indices when robuts_buffer_access is enabled
On May 19, 2016 12:50 AM, "Michael Schellenberger Costa" <
mschellenbergerco...@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_
Re: [Mesa-dev] [PATCH v2 4/4] anv/pipeline: Bounds-check resource indices when robuts_buffer_access is enabled
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
| 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) {
>unsigne
