[Mesa-dev] [PATCH 3/5] i965/fs: New peephole optimization to generate SEL.

[Matt Turner]

On Mon, Nov 4, 2013 at 11:30 AM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 4 November 2013 11:23, Matt Turner <mattst88 at gmail.com> wrote:
>>
>> On Mon, Nov 4, 2013 at 10:57 AM, Paul Berry <stereotype441 at gmail.com>
>> wrote:
>> > On 4 November 2013 10:31, Eric Anholt <eric at anholt.net> wrote:
>> >>
>> >> Matt Turner <mattst88 at 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.
>
>
> 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).

That seems easy enough to do. Since we're already generating the CMP
to write the flag register, we can just use it directly for the IF.
Thanks for pointing that out.