<div dir="ltr">On 4 November 2013 10:31, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im">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 </div><div><div class="h5">> +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); > + } </div></div>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. </blockquote><div> </div><div>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> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I'd be fine with either adding it to the malformed mov check, or just dropping support for embedded comparisons. _______________________________________________ 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> </div></div>