[Mesa-dev] [PATCH 07/23] i965/fs: fix copy propagation of partially invalidated entries

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> From: Iago Toral Quiroga <itoral at igalia.com>
>
> We were not invalidating entries with a src that reads more than one register
> when we find writes that overwrite any register read by entry->src after
> the first. This leads to incorrect copy propagation because we re-use
> entries from the ACP that have been partially invalidated. Same thing for
> entries with a dst that writes to more than one register.
> ---
>  .../drivers/dri/i965/brw_fs_copy_propagation.cpp   | 35 ++++++++++++++++------
>  1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> index 23df877..fe37676 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -44,6 +44,7 @@ struct acp_entry : public exec_node {
>     fs_reg dst;
>     fs_reg src;
>     uint8_t regs_written;
> +   uint8_t regs_read;
>     enum opcode opcode;
>     bool saturate;
>  };
> @@ -768,18 +769,32 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, bblock_t *block,
>        /* kill the destination from the ACP */
>        if (inst->dst.file == VGRF) {
>           foreach_in_list_safe(acp_entry, entry, &acp[inst->dst.nr % ACP_HASH_SIZE]) {
> -	    if (inst->overwrites_reg(entry->dst)) {
> -	       entry->remove();
> -	    }
> -	 }
> +            fs_reg tmp = entry->dst;
> +            for (int n = 0; n < entry->regs_written; n++) {
> +               if (inst->overwrites_reg(tmp)) {
> +                  entry->remove();
> +                  break;
> +               }
> +               tmp.reg_offset++;
> +            }
> +         }

The loop shouldn't be necessary, I suggest we do something like:

|  if (regions_overlap(entry->dst, entry->regs_written,
|                      inst->dst, inst->regs_written) {
|     entry->remove();
|     break;
|  }

where:

| inline bool
| regions_overlap(const fs_reg &r, unsigned n, const fs_reg &s, unsigned m)
| {
|     return r.file == s.file && r.nr == s.nr &&
|            !(r.reg_offset + n < s.reg_offset ||
|              s.reg_offset + m < r.reg_offset);
| }

Alternatively you could extend 'fs_inst::overwrites_reg()' to take a
register range instead of a single register -- Whatever you do it would
be nice to see a v2 of the patch before you push.

>  
>           /* Oops, we only have the chaining hash based on the destination, not
>            * the source, so walk across the entire table.
>            */
>           for (int i = 0; i < ACP_HASH_SIZE; i++) {
>              foreach_in_list_safe(acp_entry, entry, &acp[i]) {
> -               if (inst->overwrites_reg(entry->src))
> -                  entry->remove();
> +               /* Make sure we kill the entry if this instructions overwrites
> +                * _any_ of the registers that it reads
> +                */
> +               fs_reg tmp = entry->src;
> +               for (int n = 0; n < entry->regs_read; n++) {
> +                  if (inst->overwrites_reg(tmp)) {
> +                     entry->remove();
> +                     break;
> +                  }
> +                  tmp.reg_offset++;
> +               }

Use 'regions_overlap' here too instead of a loop.

>              }
>  	 }
>        }
> @@ -788,10 +803,11 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, bblock_t *block,
>         * operand of another instruction, add it to the ACP.
>         */
>        if (can_propagate_from(inst)) {
> -	 acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
> -	 entry->dst = inst->dst;
> -	 entry->src = inst->src[0];
> +         acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
> +         entry->dst = inst->dst;
> +         entry->src = inst->src[0];
>           entry->regs_written = inst->regs_written;
> +         entry->regs_read = inst->regs_read(0);
>           entry->opcode = inst->opcode;
>           entry->saturate = inst->saturate;
>           acp[entry->dst.nr % ACP_HASH_SIZE].push_tail(entry);
> @@ -807,6 +823,7 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, bblock_t *block,
>                 entry->dst.reg_offset = offset;
>                 entry->src = inst->src[i];
>                 entry->regs_written = regs_written;
> +               entry->regs_read = inst->regs_read(i);
>                 entry->opcode = inst->opcode;
>                 if (!entry->dst.equals(inst->src[i])) {
>                    acp[entry->dst.nr % ACP_HASH_SIZE].push_tail(entry);
> -- 
> 2.5.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160510/bfa5db64/attachment.sig>