[Mesa-dev] [PATCH v3] i965: Handle scratch accesses where reladdr also points to scratch space
Francisco Jerez
currojerez at riseup.net
Mon Mar 30 04:51:02 PDT 2015
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.
>
> v2 (Francisco Jerez):
> - Handle also recursive reladdr addressing.
> - Do not memcpy dst_reg into src_reg when rewriting reladdr.
>
> v3 (Francisco Jerez):
> - Reduce complexity by moving recursive reladdr scratch access handling
> to a separate recursive function.
> - Do not skip demoting reladdr index registers to scratch space if the
> top level GRF has already been visited.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89508
> ---
> src/mesa/drivers/dri/i965/brw_vec4.h | 2 +
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 116 ++++++++++++++++++++-----
> 2 files changed, 94 insertions(+), 24 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 33297ae..6ec00d5 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -367,6 +367,8 @@ public:
> dst_reg dst,
> src_reg orig_src,
> int base_offset);
> + src_reg emit_resolve_reladdr(int scratch_loc[], bblock_t *block,
> + vec4_instruction *inst, src_reg src);
>
> bool try_emit_mad(ir_expression *ir);
> bool try_emit_b2f_of_compare(ir_expression *ir);
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 26a3b9f..8e256f9 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -3352,6 +3352,48 @@ vec4_visitor::emit_scratch_write(bblock_t *block, vec4_instruction *inst,
> }
>
> /**
> + * Checks if @src and/or src.reladdr require a scratch read, and if so,
I don't think @ is valid doxygen syntax for referring to a function
parameter, use \p.
> + * adds the scratch read(s) before @inst. The function also checks for
> + * recursive reladdr scratch accesses, issuing the corresponding scratch
> + * loads and rewriting reladdr references accordingly.
> + *
> + * @return @src if it did not require a scratch load, otherwise, the
> + * register holding the result of the scratch load that the caller should
> + * use to rewrite src.
> + */
> +src_reg
> +vec4_visitor::emit_resolve_reladdr(int scratch_loc[], bblock_t *block,
> + vec4_instruction *inst, src_reg src)
> +{
> + /* First, deal with reladdr scratch accesses if needed */
> + if (src.reladdr && src.reladdr->file == GRF &&
> + scratch_loc[src.reladdr->reg] != -1) {
You just need to check src.reladdr here, the rest of the condition is
redundant.
> + /* Resolve recursive reladdr scratch access by calling ourselves
> + * with src.reladdr
> + */
> + src_reg new_reladdr = emit_resolve_reladdr(scratch_loc, block,
> + inst, *src.reladdr);
You should be able to assign the return value directly to *src.reladdr if
you make the change I suggest below.
> + /* Rewrite our reladdr with the register holding the result of
> + * our sec.reladdr scratch load
> + */
> + src.reladdr->file = new_reladdr.file;
> + src.reladdr->reg = new_reladdr.reg;
> + src.reladdr->reg_offset = new_reladdr.reg_offset;
> + src.reladdr->reladdr = NULL;
> + }
> +
> + /* Now handle scratch access on src */
> + if (src.file == GRF && scratch_loc[src.reg] != -1) {
> + dst_reg temp = dst_reg(this, glsl_type::vec4_type);
> + emit_scratch_read(block, inst, temp, src,
> + scratch_loc[src.reg]);
> + return src_reg(temp);
This should preserve the register type, flags and swizzle of src. Maybe
just modify src.reg, .reg_offset and .reladdr to point to temp as you
did elsewhere.
> + } else {
> + return src;
> + }
> +}
> +
> +/**
> * We can't generally support array access in GRF space, because a
> * single instruction's destination can only span 2 contiguous
> * registers. So, we send all GRF arrays that get variable index
> @@ -3368,20 +3410,29 @@ vec4_visitor::move_grf_array_access_to_scratch()
> * scratch.
> */
> foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
> - if (inst->dst.file == GRF && inst->dst.reladdr &&
> - scratch_loc[inst->dst.reg] == -1) {
> - scratch_loc[inst->dst.reg] = c->last_scratch;
> - c->last_scratch += this->alloc.sizes[inst->dst.reg];
> + if (inst->dst.file == GRF && inst->dst.reladdr) {
> + if (scratch_loc[inst->dst.reg] == -1) {
> + scratch_loc[inst->dst.reg] = c->last_scratch;
> + c->last_scratch += this->alloc.sizes[inst->dst.reg];
> + }
> +
> + src_reg *iter = inst->dst.reladdr;
Any reason you didn't fold this declaration into the init statement of
the for loop? It's not a big deal in this specific case but doing this
in general will extend the scope of the induction variable outside of
the loop unnecessarily.
> + for (; iter->reladdr; iter = iter->reladdr) {
> + if (iter->file == GRF && scratch_loc[iter->reg] == -1) {
> + scratch_loc[iter->reg] = c->last_scratch;
> + c->last_scratch += this->alloc.sizes[iter->reg];
> + }
> + }
> }
>
> for (int i = 0 ; i < 3; i++) {
> - src_reg *src = &inst->src[i];
> -
> - if (src->file == GRF && src->reladdr &&
> - scratch_loc[src->reg] == -1) {
> - scratch_loc[src->reg] = c->last_scratch;
> - c->last_scratch += this->alloc.sizes[src->reg];
> - }
> + src_reg *iter = &inst->src[i];
Same here.
> + for (; iter->reladdr; iter = iter->reladdr) {
> + if (iter->file == GRF && scratch_loc[iter->reg] == -1) {
> + scratch_loc[iter->reg] = c->last_scratch;
> + c->last_scratch += this->alloc.sizes[iter->reg];
> + }
> + }
> }
> }
>
> @@ -3395,23 +3446,40 @@ vec4_visitor::move_grf_array_access_to_scratch()
> base_ir = inst->ir;
> current_annotation = inst->annotation;
>
> + /* First handle scratch access on the dst. Notice we have to handle
> + * the case where the dst's reladdr also points to scratch space.
> + */
> + if (inst->dst.reladdr) {
> + src_reg temp = emit_resolve_reladdr(scratch_loc, block, inst,
> + *inst->dst.reladdr);
> + inst->dst.reladdr->file = temp.file;
> + inst->dst.reladdr->reg = temp.reg;
> + inst->dst.reladdr->reg_offset = temp.reg_offset;
> + inst->dst.reladdr->reladdr = NULL;
> + }
> +
With the change I suggested above you can cut this down to:
| if (inst->dst.reladdr)
| *inst->dst.reladdr = emit_resolve_reladdr(..., *inst->dst.reladdr);
> if (inst->dst.file == GRF && scratch_loc[inst->dst.reg] != -1) {
> - emit_scratch_write(block, inst, scratch_loc[inst->dst.reg]);
> + /* Now that we have handled any (possibly recursive) reladdr scratch
> + * accesses for dst we can safely do the scratch write for dst itself
> + */
> + emit_scratch_write(block, inst, scratch_loc[inst->dst.reg]);
> }
>
> + /* Now handle scratch access on any src. In this case, since inst->src[i]
> + * already is a src_reg, we can just call emit_resolve_reladdr with
> + * inst->src[i] and it will take care of handling scratch loads for
> + * both src and src.reladdr (recursively).
> + */
> for (int i = 0 ; i < 3; i++) {
> - if (inst->src[i].file != GRF || scratch_loc[inst->src[i].reg] == -1)
> - continue;
> -
> - dst_reg temp = dst_reg(this, glsl_type::vec4_type);
> -
> - emit_scratch_read(block, inst, temp, inst->src[i],
> - scratch_loc[inst->src[i].reg]);
> -
> - inst->src[i].file = temp.file;
> - inst->src[i].reg = temp.reg;
> - inst->src[i].reg_offset = temp.reg_offset;
> - inst->src[i].reladdr = NULL;
> + if (inst->src[i].file != GRF || scratch_loc[inst->src[i].reg] == -1)
> + continue;
> +
This check is redundant, it's already taken care of by
emit_resolve_reladdr().
> + src_reg temp = emit_resolve_reladdr(scratch_loc, block, inst,
> + inst->src[i]);
> + inst->src[i].file = temp.file;
> + inst->src[i].reg = temp.reg;
> + inst->src[i].reg_offset = temp.reg_offset;
> + inst->src[i].reladdr = NULL;
inst->src[i] = emit_resolve_reladdr(..., inst->src[i]);
> }
> }
> }
> --
> 1.9.1
-------------- 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/20150330/df9a1b77/attachment.sig>
More information about the mesa-dev
mailing list