[Mesa-dev] [PATCH v5] i965/vec4: refactor brw_vec4_copy_propagation.

[Jason Ekstrand]

On Tue, Sep 22, 2015 at 8:06 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> Now it is more similar to brw_fs_copy_propagation, with three
> clear stages:
>
> 1) Build up the value we are propagating as if it were the source of a
> single MOV:
> 2) Check that we can propagate that value
> 3) Build the final value
>
> Previously everything was somewhat messed up, making the
> implementation on some specific cases, like knowing if you can
> propagate from a previous instruction even with type mismatches even
> messier (for example, with the need of maintaining more of one
> has_source_modifiers). The refactoring clears stuff, and gives
> support to this mentioned use case without doing anything extra
> (for example, only one has_source_modifiers is used).
>
> Shader-db results for vec4 programs on Haswell:
> total instructions in shared programs: 1683842 -> 1669037 (-0.88%)
> instructions in affected programs:     739837 -> 725032 (-2.00%)
> helped:                                6237
> HURT:                                  0
>
> v2: using 'arg' index to get the from inst was wrong, as pointed
>     by Jason Ekstrand
> v3: rebased against last change on the previous patch of the series
> v4: don't need to track instructions on struct copy_entry, as we
>     only set the source on a direct copy, as pointed by Jason
> v5: change the approach for a refactoring, as pointed by Jason
> ---
>
> Just the refactoring suggested at v4 review is enough to get the use
> case was dealing with at the beginning solved. It would be hard
> to split this patch in two (refactoring+solve issue).
>
> Tested with piglit without any regression. Needed to update shader-db
> numbers after last Matt Turner's improvements. I think that the fact
> of going from 30 HURT (v4) to 0 (v5) is Matt's merit.
>
> I added comments to clearly mark the different stages mentioned at
> v4 review (1, 2 and 3), to make the review easier, but if the patch
> gets approved, they can go away.

I'd like to keep them only without the "1)", "2)", etc.

> The change itself doesn't explain clearly how it got solved, but the
> resulting code is clearer. Thanks for the thoroughly review.
>
>  .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 28 ++++++++++++++--------
>  1 file changed, 18 insertions(+), 10 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 1522eea..8a0bd72 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -265,6 +265,9 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>                     vec4_instruction *inst,
>                     int arg, struct copy_entry *entry)
>  {
> +   /* 1) Build up the value we are propagating as if it were the source of a
> +    * single MOV
> +    */
>     /* For constant propagation, we only handle the same constant
>      * across all 4 channels.  Some day, we should handle the 8-bit
>      * float vector format, which would let us constant propagate

Can we please kill this comment while we're here?  It seems to have
gotten copied+pasted from constant propagation and makes no sense in
this context.

With that, and the updated comments,

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

I'm also running it through our CI system as I type.  I'll let you
know how that goes when I get to my office in about an hour.

Do you have push access yet?  If not, I can make the trivial changes
and push this for you.
--Jason

> @@ -291,9 +294,9 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>     for (int i = 0; i < 4; i++) {
>        s[i] = BRW_GET_SWZ(entry->value[i]->swizzle, i);
>     }
> -   value.swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
> -                                       BRW_SWIZZLE4(s[0], s[1], s[2], s[3]));
> +   value.swizzle = BRW_SWIZZLE4(s[0], s[1], s[2], s[3]);
>
> +   /* 2) Check that we can propagate that value */
>     if (value.file != UNIFORM &&
>         value.file != GRF &&
>         value.file != ATTR)
> @@ -304,13 +307,6 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>        return false;
>     }
>
> -   if (inst->src[arg].abs) {
> -      value.negate = false;
> -      value.abs = true;
> -   }
> -   if (inst->src[arg].negate)
> -      value.negate = !value.negate;
> -
>     bool has_source_modifiers = value.negate || value.abs;
>
>     /* gen6 math and gen7+ SENDs from GRFs ignore source modifiers on
> @@ -376,6 +372,16 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>        }
>     }
>
> +   /* 3) Build the final value */
> +   if (inst->src[arg].abs) {
> +      value.negate = false;
> +      value.abs = true;
> +   }
> +   if (inst->src[arg].negate)
> +      value.negate = !value.negate;
> +
> +   value.swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
> +                                       value.swizzle);
>     if (has_source_modifiers &&
>         value.type != inst->src[arg].type) {
>        /* We are propagating source modifiers from a MOV with a different
> @@ -387,8 +393,10 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>           inst->src[i].type = value.type;
>        }
>        inst->dst.type = value.type;
> -   } else
> +   } else {
>        value.type = inst->src[arg].type;
> +   }
> +
>     inst->src[arg] = value;
>     return true;
>  }
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev