[Mesa-dev] [PATCH v2 07/30] 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.
>
> v2 (Sam):
> - Improve code by defining regions_overlap() and using it instead of a
> loop (Curro).

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> ---
>  .../drivers/dri/i965/brw_fs_copy_propagation.cpp   | 35 +++++++++++++++++-----
>  1 file changed, 27 insertions(+), 8 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 bd56e13..e9ed991 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;
>  };
> @@ -702,6 +703,14 @@ can_propagate_from(fs_inst *inst)
>             !inst->is_partial_write());
>  }
>  
> +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);
> +}
> +
>  /* Walks a basic block and does copy propagation on it using the acp
>   * list.
>   */
> @@ -728,18 +737,26 @@ 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();
> -	    }
> -	 }
> +            if (regions_overlap(entry->dst, entry->regs_written,
> +                                inst->dst, inst->regs_written)) {
> +               entry->remove();
> +               break;
> +            }
> +         }
>  
>           /* 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))
> +               /* Make sure we kill the entry if this instruction overwrites
> +                * _any_ of the registers that it reads
> +                */
> +               if (regions_overlap(entry->src, entry->regs_read,
> +                                   inst->dst, inst->regs_written)) {
>                    entry->remove();
> +                  continue;
> +               }
>              }
>  	 }
>        }
> @@ -748,10 +765,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);
> @@ -769,6 +787,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/20160512/0324ed83/attachment.sig>