[Mesa-dev] [PATCH] i965/fs: Clamp image array indices to the array bounds on IVB.

[Timothy Arceri]

On Wed, 2015-08-12 at 13:47 +0300, Francisco Jerez wrote:
> Timothy Arceri <t_arceri at yahoo.com.au> writes:
> 
> > On Mon, 2015-07-27 at 16:01 +0300, Francisco Jerez wrote:
> > > This fixes the spec at 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;
> > >        }
> > >     }