On Wed, 2015-08-12 at 13:47 +0300, Francisco Jerez wrote: > Timothy Arceri <t_arc...@yahoo.com.au> writes: > > > On Mon, 2015-07-27 at 16:01 +0300, Francisco Jerez wrote: > > > This fixes the spec@arb_shader_image_load_store@invalid index bounds > > > piglit tests on IVB, which were causing a GPU hang and then a crash > > > due to the invalid binding table index result of the array index > > > calculation. Other generations seem to behave sensibly when an > > > invalid surface is provided so it doesn't look like we need to care. > > > --- > > > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 25 +++++++++++++++++++++---- > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > > index 1671595..b69e96b 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > > > @@ -1216,14 +1216,31 @@ fs_visitor::get_nir_image_deref(const > > > nir_deref_var > > > *deref) > > > nir_deref_as_array(deref->deref.child); > > > assert(deref->deref.child->deref_type == nir_deref_type_array && > > > deref_array->deref.child == NULL); > > > + const unsigned size = glsl_get_length(deref->var->type); > > > + const unsigned base = MIN2(deref_array->base_offset, size - 1); > > > > > > - image = offset(image, bld, > > > - deref_array->base_offset * BRW_IMAGE_PARAM_SIZE); > > > + image = offset(image, bld, base * BRW_IMAGE_PARAM_SIZE); > > > > > > if (deref_array->deref_array_type == > > > nir_deref_array_type_indirect) { > > > fs_reg *tmp = new(mem_ctx) fs_reg(vgrf(glsl_type::int_type)); > > > - bld.MUL(*tmp, get_nir_src(deref_array->indirect), > > > - fs_reg(BRW_IMAGE_PARAM_SIZE)); > > > + > > > + if (devinfo->gen == 7 && !devinfo->is_haswell) { > > > + /* IVB hangs when trying to access an invalid surface index > > > with > > > + * the dataport. According to the spec "if the index used > > > to > > > + * select an individual element is negative or greater than > > > or > > > + * equal to the size of the array, the results of the > > > operation > > > + * are undefined but may not lead to termination" -- which > > > is > > > one > > > + * of the possible outcomes of the hang. Clamp the index > > > to > > > + * prevent access outside of the array bounds. > > > + */ > > > + bld.emit_minmax(*tmp, retype(get_nir_src(deref_array > > > ->indirect), > > > + BRW_REGISTER_TYPE_UD), > > > + fs_reg(size - base - 1), > > > BRW_CONDITIONAL_L); > > > > I know this was just pushed but I'm working on AoA support for this, and > > notic > > ed something. Isn't base always going to be 0 in the above line? As it > > will > > only be set to another value for direct indexing. > > > Currently it may be, but AFAIK the offset of an indirectly-addressed > array in NIR is supposed to be given by base_offset plus the indirect > offset, and none of the surrounding back-end code seemed to make the > assumption that base_offset would be zero in the indirect case, so it > didn't seem reasonable to me to do it here.
Ahh, right I see the comment in nir.h now. In practice it seems this only happens for Mesa IR. Anyway I'm sure its fine as is, thanks for the explaination. > > > > > > + } else { > > > + bld.MOV(*tmp, get_nir_src(deref_array->indirect)); > > > + } > > > + > > > + bld.MUL(*tmp, *tmp, fs_reg(BRW_IMAGE_PARAM_SIZE)); > > > image.reladdr = tmp; > > > } > > > } _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev