[Mesa-dev] [PATCH 2/2] i965/vec4: Change types as needed to propagate source modifiers using from instruction

[Jason Ekstrand]

On Wed, Sep 16, 2015 at 12:47 PM, Alejandro Piñeiro
<apinheiro at igalia.com> wrote:
> SEL and MOV instructions, as long as they don't have source modifiers, are
> just copying bits around.  So those kind of instruction could be propagated
> even if there are type mismatches. This is needed because NIR generates
> integer SEL and MOV instructions whenever it doesn't know what else to
> generate.
>
> This commit adds support for copy propagation using previous instruction
> as reference.
> ---
>
> I was tempted to try to remove copy_entry->value, as with this commit
> we are tracking the instructions too, but I think that the code would
> be clearer this way.
>
>
>  .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 35 +++++++++++++++-------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> index 64e2528..f8ecd0b 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -39,6 +39,7 @@ namespace brw {
>
>  struct copy_entry {
>     src_reg *value[4];
> +   vec4_instruction *inst[4];
>     int saturatemask;
>  };
>
> @@ -320,7 +321,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>
>     if (has_source_modifiers &&
>         value.type != inst->src[arg].type &&
> -       !can_change_source_types(inst))
> +       !can_change_source_types(inst) &&
> +       !can_change_source_types(entry->inst[arg]))

This isn't right.  The entry->inst array is indexed on vec4 component
but arg is the argument of the instruction.  I think what you want to
do is add something to the loop above to loop over 0...3 and check
them all.  Also, how this is different from has_source_modifiers?
Obviously, it is (the shader-db numbers say so) but I'm not seeing it.
Could you please provide an example.

>        return false;
>
>     if (has_source_modifiers &&
> @@ -338,7 +340,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>      * instead. See also resolve_ud_negate().
>      */
>     if (value.negate &&
> -       value.type == BRW_REGISTER_TYPE_UD)
> +       value.type == BRW_REGISTER_TYPE_UD &&
> +       !can_change_source_types(entry->inst[arg]))
>        return false;
>
>     /* Don't report progress if this is a noop. */
> @@ -376,17 +379,25 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>
>     if (has_source_modifiers &&
>         value.type != inst->src[arg].type) {
> -      /* We are propagating source modifiers from a MOV with a different
> -       * type.  If we got here, then we can just change the source and
> -       * destination types of the instruction and keep going.
> +      /* We are propagating source modifiers from a safe instruction with a
> +       * different type. If we got here, then we can just change the source
> +       * and destination types of the current instruction or the instruction
> +       * from we are propagating.
>         */
> -      assert(can_change_source_types(inst));
> -      for (int i = 0; i < 3; i++) {
> -         inst->src[i].type = value.type;
> +      assert(can_change_source_types(inst) ||
> +             can_change_source_types(entry->inst[arg]));
> +
> +      if (can_change_source_types(inst)) {
> +         for (int i = 0; i < 3; i++) {
> +            inst->src[i].type = value.type;
> +         }
> +         inst->dst.type = value.type;
> +      } else {
> +         value.type = inst->src[arg].type;
>        }
> -      inst->dst.type = value.type;
> -   } else
> +   } else {
>        value.type = inst->src[arg].type;
> +   }
>     inst->src[arg] = value;
>     return true;
>  }
> @@ -439,7 +450,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
>          for (c = 0; c < 4; c++) {
>              int channel = BRW_GET_SWZ(inst->src[i].swizzle, c);
>              entry.value[c] = entries[reg].value[channel];
> -
> +            entry.inst[c] = entries[reg].inst[channel];
>             /* If there's no available copy for this channel, bail.
>              * We could be more aggressive here -- some channels might
>              * not get used based on the destination writemask.
> @@ -484,6 +495,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
>                 entries[reg].value[i] = direct_copy ? &inst->src[0] : NULL;
>                 entries[reg].saturatemask |=
>                    inst->saturate && direct_copy ? 1 << i : 0;
> +               entries[reg].inst[i] = direct_copy ? inst : NULL;
>              }
>          }
>
> @@ -498,6 +510,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
>                   if (is_channel_updated(inst, entries[i].value, j)) {
>                      entries[i].value[j] = NULL;
>                      entries[i].saturatemask &= ~(1 << j);
> +                     entries[i].inst[j] = NULL;
>                    }
>                }
>             }
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev