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

[Juha-Pekka Heikkilä]

I did today run these patches against piglit glsl tests and there was
no regressions. I did go testing write the attached patch on top of
this set, it change MULs with explicit accumulator write into implicit
accumulator write. I guess what Eric said mean something like the
attached patch is needed?

In the patch I also did look into MACH command I did comment about but
was not certain what is the easiest way to see the effect. I started
looking at the changes in generated opcodes I might cause by setting
"INTEL_DEBUG=fs", and do comparison on the files I get, is this good
way to do it or is there some more convenient way?

On Thu, Apr 10, 2014 at 3:48 PM, Juha-Pekka Heikkilä
<juhapekka.heikkila at gmail.com> wrote:
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-i965-Change-gpu-opdoces-which-implicitly-write-accum.patch
Type: text/x-diff
Size: 5086 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140411/592031a1/attachment.patch>