On Tue, Sep 22, 2015 at 9:17 AM, Alejandro Piñeiro <apinhe...@igalia.com> wrote:
>
>
> On 22/09/15 18:09, Jason Ekstrand wrote:
>> 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.
>
> Ok.
>
>>> 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.
>
> Ok.
>
>>
>> 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.
>
> Ok. I will wait before pushing. I should be still around.
>
>> Do you have push access yet? If not, I can make the trivial changes
>> and push this for you.
>> --Jason
>
> Yes, I have push access, so I can make the changes myself. As mentioned
> I will wait for the CI system outcome.
CI gives the green light. Push it! Thanks for all your hard work and patience!
--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
>
> --
> Alejandro Piñeiro (apinhe...@igalia.com)
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
