[Mesa-dev] [PATCH] i965: Handle scratch accesses where reladdr also points to scratch space

[Iago Toral]

On Fri, 2015-03-13 at 16:29 +0200, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > This is a problem when we have IR like this:
> >
> > (array_ref (var_ref temps) (swiz x (expression ivec4 bitcast_f2i
> >    (swiz xxxx (array_ref (var_ref temps) (constant int (2)) ) )) )) ) )
> >
> > where we are indexing an array with the result of an expression that
> > accesses the same array.
> >
> > In this scenario, temps will be moved to scratch space and we will need
> > to add scratch reads/writes for all accesses to temps, however, the
> > current implementation does not consider the case where a reladdr pointer
> > (obtained by indexing into temps trough a expression) points to a register
> > that is also stored in scratch space (as in this case, where the expression
> > used to index temps access temps[2]), and thus, requires a scratch read
> > before it is accessed.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89508
> 
> It looks like we should also handle more levels of indirection --
> E.g. what if r.reladdr->reladdr->reladdr != NULL?  I guess you could
> just wrap the whole lowering pass into a loop that keeps iterating as
> long as there are unresolved register accesses with reladdr != NULL,
> kind of like vec4_visitor::move_uniform_array_access_to_pull_constants()
> does -- That way you would completely avoid the need to special-case
> reladdr offsets being stored in scratch space, you'd just leave them for
> the next iteration to clean up.

Oh, right... I did not think of that.

> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > index 5bf9e1b..b345864 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> > @@ -3426,6 +3426,19 @@ vec4_visitor::move_grf_array_access_to_scratch()
> >        base_ir = inst->ir;
> >        current_annotation = inst->annotation;
> >  
> > +      /* If our reladdr points to scratch space, then we have to load
> > +       * it too before we use it
> > +       */
> > +      if (inst->dst.reladdr &&
> > +          inst->dst.reladdr->file == GRF &&
> 
> > +          inst->dst.reladdr->reg < this->alloc.count &&
> 
> This check is unnecessary.
> 
> > +          scratch_loc[inst->dst.reladdr->reg] != -1) {
> 
> This doesn't guarantee that all cases with relative reladdr have
> actually been demoted to scratch space by the first iteration above,
> i.e. we need to make sure that for all registers r with
> r.reladdr->reladdr != NULL we're actually allocating scratch space for
> r.reladdr.

Yeah, you are right.

> > +         dst_reg temp = dst_reg(this, glsl_type::vec4_type);
> > +         emit_scratch_read(block, inst, temp, *inst->dst.reladdr,
> > +                           scratch_loc[inst->dst.reladdr->reg]);
> > +         memcpy(inst->dst.reladdr, &temp, sizeof(temp));
> 
> This is memcpy-ing a dst_reg into a src_reg, you need an actual
> conversion, please use the assignment operator instead of memcpy() for
> type safety.

Ooops!

Thanks for reviewing Francisco, I'll address the comments and send a new
patch.

Iago

> > +      }
> > +
> >        if (inst->dst.file == GRF && scratch_loc[inst->dst.reg] != -1) {
> >  	 emit_scratch_write(block, inst, scratch_loc[inst->dst.reg]);
> >        }
> > @@ -3434,6 +3447,19 @@ vec4_visitor::move_grf_array_access_to_scratch()
> >  	 if (inst->src[i].file != GRF || scratch_loc[inst->src[i].reg] == -1)
> >  	    continue;
> >  
> > +         /* If our reladdr points to scratch space, then we have to load
> > +          * it too before we use it
> > +          */
> > +         if (inst->src[i].reladdr &&
> > +             inst->src[i].reladdr->file == GRF &&
> > +             inst->src[i].reladdr->reg < this->alloc.count &&
> > +             scratch_loc[inst->src[i].reladdr->reg] != -1) {
> > +            dst_reg temp = dst_reg(this, glsl_type::vec4_type);
> > +            emit_scratch_read(block, inst, temp, *inst->src[i].reladdr,
> > +                              scratch_loc[inst->src[i].reladdr->reg]);
> > +            memcpy(inst->src[i].reladdr, &temp, sizeof(temp));
> 
> Same here.
> 
> > +         }
> > +
> >  	 dst_reg temp = dst_reg(this, glsl_type::vec4_type);
> >  
> >  	 emit_scratch_read(block, inst, temp, inst->src[i],
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev