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

[Alejandro Piñeiro]

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.

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