On 22/09/15 19:27, Jason Ekstrand wrote: > On Tue, Sep 22, 2015 at 9:17 AM, Alejandro Piñeiro <[email protected]> > wrote: >> >> On 22/09/15 18:09, Jason Ekstrand wrote: >>> On Tue, Sep 22, 2015 at 8:06 AM, Alejandro Piñeiro <[email protected]> >>> 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 <[email protected]> >>> >>> 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 ([email protected]) _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
