On Fri, 2015-03-13 at 16:29 +0200, Francisco Jerez wrote:
> Iago Toral Quiroga <ito...@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
