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

[Jason Ekstrand]

On Thu, Sep 24, 2015 at 9:29 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
>
> v3:
> - Move the check back to the original place, to keep things
>   tidy, as suggested by Jason Ekstrand
> ---
>
>> 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.
>
> The problem is that at the end of try_copy_propagate we are not only
> creating the entire final value, but also modifying some of the
> instruction properties, based on the fact that it is safe at that
> point.
>
>> If you don't like that, then I'd rather just go with the first patch.
>
> This commit is basically the original one, but with some cleanings,
> including Matt's suggestion on the if, plus calling compose_swizzle
> just once. I personally find more readable this way, sorry for the noise
> of asking another patch review.
>
>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 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..158b25d 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -325,7 +325,11 @@ 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)
> +   unsigned composed_swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
> +                                                   value.swizzle);
> +   if (inst->is_3src() &&
> +       value.file == UNIFORM &&
> +       !composed_swizzle)

Where did the is_single_value_swizzle() go?

>        return false;
>
>     if (inst->is_send_from_grf())
> @@ -380,8 +384,7 @@ 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);
> +   value.swizzle = composed_swizzle;
>     if (has_source_modifiers &&
>         value.type != inst->src[arg].type) {
>        assert(can_change_source_types(inst));
> --
> 2.1.4
>