[Mesa-dev] [PATCH v2] i965/vec4: check swizzle before discarding a uniform on a 3src operand

[Jason Ekstrand]

On Thu, Sep 24, 2015 at 1:32 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> Without this commit, copy propagation is discarded if it involves
> a uniform with an instruction that has 3 sources. But 3 sourced
> instructions can access scalar values.
>
> For example, this is what vec4_visitor::fix_3src_operand() is already
> doing:
>
>    if (src.file == UNIFORM && brw_is_single_value_swizzle(src.swizzle))
>       return src;
>
> Shader-db results (unfiltered) on NIR:
> total instructions in shared programs: 6259650 -> 6241985 (-0.28%)
> instructions in affected programs:     812755 -> 795090 (-2.17%)
> helped:                                7930
> HURT:                                  0
>
> Shader-db results (unfiltered) on IR:
> total instructions in shared programs: 6445822 -> 6441788 (-0.06%)
> instructions in affected programs:     296630 -> 292596 (-1.36%)
> helped:                                2533
> HURT:                                  0
>
> v2:
> - Updated commit message, using Matt Turner suggestions
> - Move the check after we've created the final value, as Jason
>   Ekstrand suggested
> - Clean up the condition
> ---
>
> Removed RFC as both Matt Turner and Jason Ekstrand accepted the change.
>
> I updated the patch as the condition was just ugly. As I made that change
> I also included Jason suggestion to move the check after the swizzle
> on the final value is computed. I know that the first version got a
> "Reviewed" by Jason, but due all these change, I prefer to be conservative,
> and ask for a new review.
>
> Thanks in advance.
>
>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 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 d3f0ddd..ce4d39d 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -325,9 +325,6 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>         inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_WRITE)
>        return false;
>
> -   if (inst->is_3src() && value.file == UNIFORM)
> -      return false;
> -
>     if (inst->is_send_from_grf())
>        return false;
>
> @@ -373,6 +370,13 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>     }
>
>     /* Build the final value */
> +   value.swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
> +                                       value.swizzle);

Why did you move this up here?  If we're going to have checks based on
the final value, they should probably go after we've constructed the
entire final value.  Does that seem reasonable?  So I'd put this back
and just put the UNIFORM 3-src check below it.  Otherwise, we're
breaking up the "build the final value" again.

If you don't like that, then I'd rather just go with the first patch.

With the requested reshuffle,

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

Otherwise, I gave you an R-B on the first patch so you can just use
that one if you'd like.

--Jason

> +   /* One last check based on the final swizzle */
> +   if (inst->is_3src() && value.file == UNIFORM &&
> +       !brw_is_single_value_swizzle(value.swizzle))
> +      return false;
> +
>     if (inst->src[arg].abs) {
>        value.negate = false;
>        value.abs = true;
> @@ -380,8 +384,6 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>     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) {
>        assert(can_change_source_types(inst));
> --
> 2.1.4
>