On May 19, 2016 12:50 AM, "Michael Schellenberger Costa" <<a href="mailto:mschellenbergercosta@googlemail.com">mschellenbergercosta@googlemail.com</a>> 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); > >