[Mesa-dev] [PATCH v2 2/3] i965/fs: Change SEL and MOV types as needed to propagate source modifiers

[Matt Turner]

On Fri, Apr 3, 2015 at 2:06 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> SEL and MOV instructions, as long as they don't have source modifiers, are
> just copying bits around.  This commit adds support to copy propagation to
> switch the type of a SEL or MOV instruction as needed so that it can
> propagate source modifiers.  This is needed because NIR generates integer
> SEL and MOV instructions whenver it doesn't know what else to generate.
>
> shader-db results with NIR:
> total FS instructions in shared programs: 4360910 -> 4360186 (-0.02%)
> FS instructions in affected programs:     59094 -> 58370 (-1.23%)
> helped:                                   341
> HURT:                                     0
> GAINED:                                   2
> LOST:                                     0
> ---
>  .../drivers/dri/i965/brw_fs_copy_propagation.cpp   | 33 +++++++++++++++++++---
>  1 file changed, 29 insertions(+), 4 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 e8d092c..d321509 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -275,6 +275,16 @@ is_logic_op(enum opcode opcode)
>             opcode == BRW_OPCODE_NOT);
>  }
>
> +static bool
> +can_change_source_types(fs_inst *inst)
> +{
> +   return !inst->src[0].abs && !inst->src[0].negate &&
> +          (inst->opcode == BRW_OPCODE_MOV ||
> +           (inst->opcode == BRW_OPCODE_SEL &&
> +            inst->conditional_mod == BRW_CONDITIONAL_NONE &&

I understand what this means, but I'd change this line to
inst->predicate (!= BRW_PREDICATE_NONE).

> +            !inst->src[1].abs && !inst->src[1].negate));
> +}
> +
>  bool
>  fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>  {
> @@ -346,7 +356,9 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>          type_sz(inst->src[arg].type)) % type_sz(entry->src.type) != 0)
>        return false;
>
> -   if (has_source_modifiers && entry->dst.type != inst->src[arg].type)
> +   if (has_source_modifiers &&
> +       entry->dst.type != inst->src[arg].type &&
> +       !can_change_source_types(inst))
>        return false;
>
>     if (brw->gen >= 8 && (entry->src.negate || entry->src.abs) &&
> @@ -412,9 +424,22 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>        break;
>     }
>
> -   if (!inst->src[arg].abs) {
> -      inst->src[arg].abs = entry->src.abs;
> -      inst->src[arg].negate ^= entry->src.negate;
> +   if (has_source_modifiers) {
> +      if (entry->dst.type != inst->src[arg].type) {
> +         /* We are propagating source modifiers from a MOV with a different

Like Anuj mentioned, MOV or SEL.

> +          * type.  If we got here, then we can just change the source and
> +          * destination types of the instruction and keep going.
> +          */
> +         assert(can_change_source_types(inst));
> +         for (int i = 0; i < inst->sources; i++)
> +            inst->src[i].type = entry->dst.type;

Probably put braces araound this.

> +         inst->dst.type = entry->dst.type;
> +      }
> +
> +      if (!inst->src[arg].abs) {
> +         inst->src[arg].abs = entry->src.abs;
> +         inst->src[arg].negate ^= entry->src.negate;
> +      }
>     }
>
>     return true;
> --
> 2.3.4

Seems totally believable. I'm a little surprised the shader-db results
aren't bigger. I'd like to play with it and confirm it handles what I
was seeing with SEL instructions.