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

[Francisco Jerez]

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.

> ---
>  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.

> +         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.

> +      }
> +
>        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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150313/c3583228/attachment.sig>