[Mesa-dev] [PATCH 3/7] i965/fs: Before reg alloc, schedule instructions to reduce live ranges.

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On 12/07/2012 02:58 PM, Eric Anholt wrote:
>> This came from an idea by Ben Segovia.  16-wide pixel shaders are very
>> important for latency hiding on i965, so we want to try really hard to
>> get them.  If scheduling an instruction makes some set of instructions
>> available, those are probably the ones that make the instruction's
>> result dead.  By choosing those first, we'll have a tendency to reduce
>> the amount of live data as opposed to creating more.
>>
>> Previously, we were sometimes getting this behavior out of the
>> scheduler, which was what produced the scheduler's original performance
>> wins on lightsmark.  Unfortunately, that was mostly an accident of the
>> lame instruction latency information that I had, which made it
>> impossible to fix the actual scheduling for performance.  Now that we've
>> fixed the scheduling for setup for register allocation, we can safely
>> update the latency parameters for the final schedule.
>>
>> In shader-db, we lose 37 16-wide shaders, but gain 90 new ones.  4
>> shaders that were spilling change how many registers spill, for a
>> reduction of 70/3899 instructions.
>> ---
>>   .../dri/i965/brw_fs_schedule_instructions.cpp      |   49 +++++++++++++++++---
>>   1 file changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_schedule_instructions.cpp b/src/mesa/drivers/dri/i965/brw_fs_schedule_instructions.cpp
>> index d48ad1e..3941eac 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_schedule_instructions.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_schedule_instructions.cpp
>> +         for (schedule_node *node = (schedule_node *)instructions.get_tail();
>> +              node != instructions.get_head()->prev;
>> +              node = (schedule_node *)node->prev) {
>> +            schedule_node *n = (schedule_node *)node;
>> +
>> +            if (!chosen || chosen->inst->regs_written() > 1) {
>> +               chosen = n;
>> +               if (chosen->inst->regs_written() <= 1)
>> +                  break;
>> +            }
>
> I don't think the if condition is necessary here.  Just doing
>
> for (...) {
> 	chosen = (schedule_node *) node;
> 	if (chosen->inst->regs_written() <= 1)
> 		break;
> }
>
> should accomplish the same thing.

Yeah, it was a leftover of trying a bunch of more complicated
heuristics.

>> -	 if (!chosen || n->unblocked_time < chosen_time) {
>> -	    chosen = n;
>> -	    chosen_time = n->unblocked_time;
>> -	 }
>> +         chosen_time = chosen->unblocked_time;
>
> It seems plausible that there could be no nodes to schedule...which 
> means chosen would be NULL here.  Perhaps just move chosen_time = 
> chosen->unblocked_time into the if...break above.

This is all in a loop while (!instructions.is_empty()), and these loops
have to pick one of those.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20121211/5f8ebc8e/attachment.pgp>