On 31 August 2012 11:32, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/mesa/drivers/dri/i965/brw_eu_compact.c | 111 ++++++++++++++++++++++------ 1 file changed, 87 insertions(+), 24 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c index 2c15c85..dd661f5 100644 --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c @@ -324,6 +324,19 @@ brw_try_compact_instruction(struct brw_compile *c, return false; /* FINISHME: What else needs handling? */ } + + if (src->header.opcode == BRW_OPCODE_IF || + src->header.opcode == BRW_OPCODE_ELSE || + src->header.opcode == BRW_OPCODE_ENDIF || + src->header.opcode == BRW_OPCODE_DO || + src->header.opcode == BRW_OPCODE_HALT || + src->header.opcode == BRW_OPCODE_WHILE) { + /* FINISHME: The fixup code below, and brw_set_uip_jip and friends, needs + * to be able to handle flow control. + */ + return false; + } + /* FINISHME: immediates */ if (src->bits1.da1.src0_reg_file == BRW_IMMEDIATE_VALUE || src->bits1.da1.src1_reg_file == BRW_IMMEDIATE_VALUE) @@ -476,12 +489,40 @@ void brw_debug_compact_uncompact(struct intel_context *intel, } } +static int +compressed_between(int old_ip, int old_target_ip, int *compressed_counts) +{ + int this_compressed_count = compressed_counts[old_ip]; + int target_compressed_count = compressed_counts[old_target_ip]; + return target_compressed_count - this_compressed_count; +} + +static void +update_uip_jip(struct brw_instruction *insn, int this_old_ip, + int *compressed_counts) +{ + int target_old_ip; + + target_old_ip = this_old_ip + insn->bits3.break_cont.jip; + insn->bits3.break_cont.jip -= compressed_between(this_old_ip, + target_old_ip, + compressed_counts); + + target_old_ip = this_old_ip + insn->bits3.break_cont.uip; + insn->bits3.break_cont.uip -= compressed_between(this_old_ip, + target_old_ip, + compressed_counts); +} + void brw_compact_instructions(struct brw_compile *p) { struct brw_context *brw = p->brw; struct intel_context *intel = &brw->intel; void *store = p->store; + /* Control flow fixup information */ + int compressed_counts[p->next_insn_offset / 8]; </blockquote><div> A comment would have saved me some time in understanding what information is stored in compressed_counts. I think this would be a correct description but I'm not certain: "If an instruction was located at offset 8*i before compaction, compressed_counts[i] is the number of instructions that preceded it which were not previously compressed, but are now." An alternative way of representing the same information, possibly easier to follow, would be: /* If an instruction was located at offset 8*i before compaction, 8*new_ip[i] is the location of the instruction after compaction */ int new_ip[p->next_insn_offset / 8]; (Of course, update_uip_jip() would need to be reworked) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + int old_ip[p->next_insn_offset / 8]; </blockquote><div> And, assuming I'm understanding the code correctly, I believe old_ip could be commented: /* If an instruction is located at offset 8*i after compaction, 8*old_ip[i] was the location of the instruction before compaction. In the case of NOPs inserted automatically for alignment, 8*old_ip[i] was the old location of the instruction that needed to be aligned. */ </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> assert(gen6_control_index_table[ARRAY_SIZE(gen6_control_index_table) - 1] != 0); assert(gen6_datatype_table[ARRAY_SIZE(gen6_datatype_table) - 1] != 0); @@ -491,36 +532,21 @@ brw_compact_instructions(struct brw_compile *p) if (intel->gen != 6) return; - /* FINISHME: If we are going to compress instructions between flow control, - * we have to do fixups to flow control offsets to represent the new - * distances, since flow control uses (virtual address distance)/2, not a - * logical instruction count. - */ - bool continue_compressing = true; - for (int i = 0; i < p->nr_insn; i++) { - if (p->store[i].header.opcode == BRW_OPCODE_WHILE) - return; - } - int src_offset; int offset = 0; + int compressed_count = 0; for (src_offset = 0; src_offset < p->nr_insn * 16;) { struct brw_instruction *src = store + src_offset; void *dst = store + offset; - switch (src->header.opcode) { - case BRW_OPCODE_IF: - case BRW_OPCODE_HALT: - case BRW_OPCODE_JMPI: - continue_compressing = false; - break; - } + old_ip[offset / 8] = src_offset / 8; + compressed_counts[src_offset / 8] = compressed_count; struct brw_instruction saved = *src; - if (continue_compressing && - !src->header.cmpt_control && + if (!src->header.cmpt_control && brw_try_compact_instruction(p, dst, src)) { + compressed_count++; /* debug */ struct brw_instruction uncompacted; @@ -544,6 +570,7 @@ brw_compact_instructions(struct brw_compile *p) align->dw0.opcode = BRW_OPCODE_NOP; align->dw0.cmpt_ctrl = 1; offset += 8; + old_ip[offset / 8] = src_offset / 8; dst = store + offset; } @@ -558,20 +585,56 @@ brw_compact_instructions(struct brw_compile *p) } } </blockquote><div> I'm running out of time to look at this today, so I had to stop reviewing here. I think the stuff that follows is likely correct (because if it weren't, it would probably fail spectacularly :)). I'll try to return to it tomorrow just to make sure. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + /* Fix up control flow offsets. */ + p->next_insn_offset = offset; + for (offset = 0; offset < p->next_insn_offset;) { + struct brw_instruction *insn = store + offset; + int this_old_ip = old_ip[offset / 8]; + int this_compressed_count = compressed_counts[this_old_ip]; + int target_old_ip, target_compressed_count; + + switch (insn->header.opcode) { + case BRW_OPCODE_BREAK: + case BRW_OPCODE_CONTINUE: + case BRW_OPCODE_HALT: + update_uip_jip(insn, this_old_ip, compressed_counts); + break; + + case BRW_OPCODE_IF: + case BRW_OPCODE_ELSE: + case BRW_OPCODE_ENDIF: + case BRW_OPCODE_WHILE: + if (intel->gen == 6) { + target_old_ip = this_old_ip + insn->bits1.branch_gen6.jump_count; + target_compressed_count = compressed_counts[target_old_ip]; + insn->bits1.branch_gen6.jump_count -= (target_compressed_count - + this_compressed_count); + } else { + update_uip_jip(insn, this_old_ip, compressed_counts); + } + break; + } + + if (insn->header.cmpt_control) { + offset += 8; + } else { + offset += 16; + } + } + /* p->nr_insn is counting the number of uncompacted instructions still, so * divide. We do want to be sure there's a valid instruction in any * alignment padding, so that the next compression pass (for the FS 8/16 * compile passes) parses correctly. */ - if (offset & 8) { + if (p->next_insn_offset & 8) { struct brw_compact_instruction *align = store + offset; memset(align, 0, sizeof(*align)); align->dw0.opcode = BRW_OPCODE_NOP; align->dw0.cmpt_ctrl = 1; - offset += 8; + p->next_insn_offset += 8; } - p->next_insn_offset = offset; - p->nr_insn = offset / 16; + p->nr_insn = p->next_insn_offset / 16; if (0) { fprintf(stdout, "dumping compacted program\n"); -- 1.7.10.4 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>