On Thu, Apr 10, 2014 at 1:18 AM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 04/09/2014 01:47 PM, Matt Turner wrote:
>> From: Juha-Pekka Heikkila <juhapekka.heikk...@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 <matts...@gmail.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikk...@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
