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

[Alejandro Piñeiro]

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)
       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