On Mon, Nov 4, 2013 at 10:57 AM, Paul Berry <stereotype...@gmail.com> wrote:
> On 4 November 2013 10:31, Eric Anholt <e...@anholt.net> wrote:
>>
>> Matt Turner <matts...@gmail.com> 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.
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.
Since the next patch didn't make a difference when pulling MOVs out
starting from the IF instruction, I'll just drop that patch.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
