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

[Paul Berry]

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).


>
> Since the next patch didn't make a difference when pulling MOVs out
> starting from the IF instruction, I'll just drop that patch.
>

Fair enough.  Either way is fine by me.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131104/0fe87f0d/attachment-0001.html>