[Mesa-dev] [PATCH 18/20] i965/fs: Preserve CFG in the SEL peephole.

[Pohjolainen, Topi]

On Tue, Aug 19, 2014 at 10:19:42AM +0300, Pohjolainen, Topi wrote:
> On Mon, Aug 18, 2014 at 10:42:26AM -0700, Matt Turner wrote:
> > On Mon, Aug 18, 2014 at 8:34 AM, Pohjolainen, Topi
> > <topi.pohjolainen at intel.com> wrote:
> > > On Thu, Jul 24, 2014 at 07:54:25PM -0700, Matt Turner wrote:
> > >> ---
> > >>  src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp | 15 +++++++++------
> > >>  1 file changed, 9 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> > >> index d64cd98..f609138 100644
> > >> --- a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> > >> +++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> > >> @@ -212,23 +212,26 @@ fs_visitor::opt_peephole_sel()
> > >>        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_inst->insert_before(block, cmp_inst);
> > >>        }
> > >>
> > >> +      bblock_t *then_block = (bblock_t *)block->link.next;
> > >> +      bblock_t *else_block = (bblock_t *)block->else_block->link.next;
> > >
> > > Isn't this a pointer to the endif-block? I thought else-block would be
> > >
> > >          bblock_t *else_block = (bblock_t *)block->then_block->link.next;
> > >
> > > or simply just
> > >
> > >          bblock_t *else_block = (bblock_t *)block->else_block;
> > 
> > It's the block immediately following the ELSE instruction (containing
> > the MOVs). E.g.,
> > 
> > B0: ...
> >     IF
> > B1: MOV
> >     MOV
> >     ELSE
> > B2: MOV
> >     MOV
> > B3: ENDIF
> >     ...
> > 
> > then_block is B1, and else_block is B2. I can name them something else
> > if that would make it clearer.
> 
> Thanks for the explanation, makes sense. I should have paid more attention to
> the context and not just stared the name. If you can come up with a better
> name though that would be nice (I cannot think of anything - "else_body_block"
> sounds awful).

And the more I thought about this, the original name is just fine. I think it
is fair to assume that anybody dealing with this code knows how instructions
are divided into blocks - else-instruction itself does not belong to the else
block (which represents the body).