[Mesa-dev] [PATCH 8/8] i965/vec4: Do copy before constant propagation after opt_vector_float().

[Matt Turner]

On Tue, Dec 23, 2014 at 11:34 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 12/23/2014 08:26 AM, Matt Turner wrote:
>> On 12/22/14, Ian Romanick <idr at freedesktop.org> wrote:
>>> On 12/21/2014 03:24 PM, Matt Turner wrote:
>>>> total instructions in shared programs: 5877012 -> 5876617 (-0.01%)
>>>> instructions in affected programs:     33140 -> 32745 (-1.19%)
>>>>
>>>> From before the commit that allows VF constant propagation (which hurt
>>>> some programs) to here, the results are:
>>>>
>>>> total instructions in shared programs: 5877951 -> 5876617 (-0.02%)
>>>> instructions in affected programs:     123444 -> 122110 (-1.08%)
>>>>
>>>> with no programs hurt.
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>>> b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>>> index d36a735..c9ba338 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>>> @@ -1800,6 +1800,7 @@ vec4_visitor::run()
>>>>
>>>>     if (opt_vector_float()) {
>>>>        opt_cse();
>>>> +      opt_copy_propagation(false);
>>>
>>> The commit messages suggest the above line should go before the
>>> opt_cse() call.
>>
>> Right, saying "after opt_cse()" seems too general though. How about:
>>
>>> i965/vec4: Do separate copy followed by constant propagation after opt_vector_float().
>
> That works for me.
>
>>>>        opt_copy_propagation();
>>>
>>> Seeing these two lines together makes the default parameter to
>>> opt_copy_propagation feel a little icky... but I guess the
>>> OPT(opt_copy_propagation()) in vec4_visitor::run won't really work
>>> otherwise.
>>>
>>> Maybe pass an explicit true here anyway?
>>
>> The OPT macro is actually variadic and passes extra arguments to the
>> optimization pass, but I think I like the default argument there, and
>> explicitly passing true/false in the opt_vector_float() block to show
>> explicitly what's going on.
>
> With the changes you've proposed here and in patch 2, the series is
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

Thanks Ian!