<div dir="ltr">On 4 November 2013 11:23, Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">On Mon, Nov 4, 2013 at 10:57 AM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > On 4 November 2013 10:31, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote: >> >> Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> writes: >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp >> > b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp >> > new file mode 100644 >> > index 0000000..9626751 >> > --- /dev/null >> > +++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp >> >> > +bool >> > +fs_visitor::opt_peephole_sel() >> > +{ >> > + bool progress = false; >> > + >> > + cfg_t cfg(this); >> > + >> > + for (int b = 0; b < cfg.num_blocks; b++) { >> > + bblock_t *block = cfg.blocks[b]; >> > + >> > + /* IF instructions, by definition, can only be found at the ends >> > of >> > + * basic blocks. >> > + */ >> > + fs_inst *if_inst = (fs_inst *) block->end; >> > + if (if_inst->opcode != BRW_OPCODE_IF) >> > + continue; >> > + >> > + if (!block->else_inst) >> > + continue; >> > + >> > + fs_inst *else_inst = (fs_inst *) block->else_inst; >> > + >> > + fs_inst *else_mov[MAX_MOVS] = { NULL }; >> > + fs_inst *then_mov[MAX_MOVS] = { NULL }; >> > + bool malformed_mov_found = false; >> > + >> > + int movs = count_movs_from_if(then_mov, else_mov, if_inst, >> > else_inst); >> > + >> > + if (movs == 0) >> > + continue; >> > + >> > + fs_inst *sel_inst[MAX_MOVS] = { NULL }; >> > + fs_inst *mov_imm_inst[MAX_MOVS] = { NULL }; >> > + >> > + /* Generate SEL instructions for pairs of MOVs to a common >> > destination. */ >> > + for (int i = 0; i < movs; i++) { >> > + if (!then_mov[i] || !else_mov[i]) >> > + break; >> > + >> > + /* Check that the MOVs are the right form. */ >> > + if (!then_mov[i]->dst.equals(else_mov[i]->dst) || >> > + then_mov[i]->is_partial_write() || >> > + else_mov[i]->is_partial_write()) { >> > + malformed_mov_found = true; >> > + break; >> > + } >> > + >> > + /* Only the last source register can be a constant, so if the >> > MOV in >> > + * the "then" clause uses a constant, we need to put it in a >> > + * temporary. >> > + */ >> > + fs_reg src0(then_mov[i]->src[0]); >> > + if (src0.file == IMM) { >> > + src0 = fs_reg(this, glsl_type::float_type); >> > + src0.type = then_mov[i]->src[0].type; >> > + mov_imm_inst[i] = MOV(src0, then_mov[i]->src[0]); >> > + } >> > + >> > + sel_inst[i] = SEL(then_mov[i]->dst, src0, >> > else_mov[i]->src[0]); >> > + >> > + if (brw->gen == 6 && if_inst->conditional_mod) { >> > + /* For Sandybridge with IF with embedded comparison */ >> > + sel_inst[i]->predicate = BRW_PREDICATE_NORMAL; >> > + } else { >> > + /* Separate CMP and IF instructions */ >> > + sel_inst[i]->predicate = if_inst->predicate; >> > + sel_inst[i]->predicate_inverse = >> > if_inst->predicate_inverse; >> > + } >> > + } >> > + >> > + if (malformed_mov_found) >> > + continue; >> > + >> > + /* Emit a CMP if our IF used the embedded comparison */ >> > + if (brw->gen == 6 && if_inst->conditional_mod) { >> > + fs_inst *cmp_inst = CMP(reg_null_d, if_inst->src[0], >> > if_inst->src[1], >> > + if_inst->conditional_mod); >> > + if_inst->insert_before(cmp_inst); >> > + } >> >> If the IF is using embedded comparison, then our MOVs that we've moved >> before it might be updating the registers used in the embedded >> comparsion. > > > I think this is ok, because we don't move any MOVs before we get to this > code. We just set up the sel_inst and mov_imm_inst arrays. The MOVs don't > get added to the instruction stream until after this code, so they appear > after the CMP, and stuff should still happen in the correct order. </div></div>I think you're right in the context of this patch, but once we start handling a a subset of the MOVs (in the next patch) we keep the if statement around and Eric's concern seems plausible. </blockquote><div> </div><div>Good point--I forgot about that. However, I think we could address Eric's concern by simply changing the embedded-comparison-if to an ordinary if; that way it will just use whatever is in the flag register (which should still be correct, since the MOVs won't affect it). </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Since the next patch didn't make a difference when pulling MOVs out starting from the IF instruction, I'll just drop that patch. </blockquote></div> </div><div class="gmail_extra">Fair enough. Either way is fine by me. </div></div>