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

[Juha-Pekka Heikkilä]

On Thu, Apr 10, 2014 at 1:18 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 04/09/2014 01:47 PM, Matt Turner wrote:
>> From: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>
>> Our hardware has an "accumulator" register, which can be used to store
>> intermediate results across multiple instructions.  Many instructions
>> can implicitly write a value to the accumulator in addition to their
>> normal destination register.  This is enabled by the "AccWrEn" flag.
>>
>> This patch introduces a new flag, inst->writes_accumulator, which
>> allows us to express the AccWrEn notion in the IR.  It also creates a
>> n ALU2_ACC macro to easily define emitters for instructions that
>> implicitly write the accumulator.
>>
>> Previously, we only supported implicit accumulator writes from the
>> ADDC, SUBB, and MACH instructions.  We always enabled them on those
>> instructions, and left them disabled for other instructions.
>>
>> To take advantage of the MAC (multiply-accumulate) instruction, we
>> need to be able to set AccWrEn on other types of instructions.
>>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>> I split out is_accumulator() into a separate patch, and made some
>> fixes to the scheduling code. Let me know if these changes look good
>> to you, JP. (Patch formatted with -U15 as to see other sections of
>> the scheduling code during review)
>>
>>  src/mesa/drivers/dri/i965/brw_fs.cpp               | 26 ++++++----
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp     |  7 +--
>>  .../drivers/dri/i965/brw_schedule_instructions.cpp | 58 ++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_shader.h             |  1 +
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp             | 15 ++----
>>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp   |  7 +--
>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp     | 17 +++++--
>>  7 files changed, 95 insertions(+), 36 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index e576545..0eece60 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> [snip]
>> @@ -2113,40 +2124,35 @@ fs_visitor::dead_code_eliminate()
>>
>>           for (int i = 0; i < inst->regs_written; i++) {
>>              int var = live_intervals->var_from_vgrf[inst->dst.reg];
>>              assert(live_intervals->end[var + inst->dst.reg_offset + i] >= pc);
>>              if (live_intervals->end[var + inst->dst.reg_offset + i] != pc) {
>>                 dead = false;
>>                 break;
>>              }
>>           }
>>
>>           if (dead) {
>>              /* Don't dead code eliminate instructions that write to the
>>               * accumulator as a side-effect. Instead just set the destination
>>               * to the null register to free it.
>>               */
>> -            switch (inst->opcode) {
>> -            case BRW_OPCODE_ADDC:
>> -            case BRW_OPCODE_SUBB:
>> -            case BRW_OPCODE_MACH:
>> +            if (inst->writes_accumulator) {
>>                 inst->dst = fs_reg(retype(brw_null_reg(), inst->dst.type));
>> -               break;
>
> Pre-existing bug: we ought to set progress = true in this case.

I am still wondering if the content of "if (inst->writes_accumulator)
{}" should be removed totally? Instead of above one would in the code
lines which emit opcodes where inst->writes_accumulator will be set
true also set the destination to brw_null_reg() if needed. I'm not
super familiar yet with all possible opcodes but at least for MACH
this should go horribly wrong here because MACH writes different
results in accumulator than destination -- and MACH will always have
writes_accumulator set to true.

>
>> -            default:
>> +            } else {
>>                 inst->remove();
>>                 progress = true;
>> -               break;
>>              }
>>           }
>>        }
>>
>>        pc++;
>>     }
>>
>>     if (progress)
>>        invalidate_live_intervals();
>>
>>     return progress;
>>  }
>>
>>  struct dead_code_hash_key
>>  {
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>