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

[Alejandro Piñeiro]

On 22/09/15 19:27, Jason Ekstrand wrote:
> On Tue, Sep 22, 2015 at 9:17 AM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
>>
>> On 22/09/15 18:09, Jason Ekstrand wrote:
>>> On Tue, Sep 22, 2015 at 8:06 AM, Alejandro Piñeiro <apinheiro at 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.ekstrand at 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

Pushed! Thanks!

-- 
Alejandro Piñeiro (apinheiro at igalia.com)