[Mesa-dev] [PATCH 1/5] i965: Add writes_accumulator flag

[Juha-Pekka Heikkilä]

Hi Matt,

the changed set looks good to me, I did side by side comparison on
what had changed but did not try to run it today. I realized
immediately when seeing your comment I had not understood to consider
the "WAR" vs. "RAW" comments in the scheduler. I was thinking when I
made the latest set the first patch had grown a bit big but did not go
braking it yet into pieces.

On Wed, Apr 9, 2014 at 11:58 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Fri, Apr 4, 2014 at 6:51 AM, Juha-Pekka Heikkila
> <juhapekka.heikkila at gmail.com> wrote:
>> diff --git a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
>> index a951459..92f82fd 100644
>> --- a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
>> @@ -758,6 +758,7 @@ fs_instruction_scheduler::calculate_deps()
>>     schedule_node *last_fixed_grf_write = NULL;
>>     int reg_width = v->dispatch_width / 8;
>>
>> +   schedule_node *last_accumulator_write = NULL;
>>     /* The last instruction always needs to still be the last
>>      * instruction.  Either it's flow control (IF, ELSE, ENDIF, DO,
>>      * WHILE) and scheduling other things after it would disturb the
>> @@ -822,6 +823,10 @@ fs_instruction_scheduler::calculate_deps()
>
> The line before this was
>   if (inst->reads_flag()) {
>>          add_dep(last_conditional_mod[inst->flag_subreg], n);
>>        }
>>
>> +      if (inst->writes_accumulator || inst->dst.is_accumulator()) {
>> +         add_dep(last_accumulator_write, n);
>> +      }
>
> But we're checking if we're writing the accumulator here, instead of reading it.
>
> We're also not giving the scheduler any benefits from it's new
> knowledge of accumulator dependencies, because we're still calling
> add_barrier_deps() above when we don't recognize the destination. I
> hope you don't mind, but I split the is_accumulator() additions into a
> separate patch, fixed up the scheduler hunks and sent the revised
> patch. Let me know if it looks right to you.