On Tue, Sep 22, 2015 at 8:06 AM, Alejandro Piñeiro <apinhe...@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.ekstr...@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
