[Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)

[Ben Widawsky]

On Wed, Nov 19, 2014 at 01:28:25AM -0800, Kenneth Graunke wrote:
> On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
> > Add support for decoding the new branch control bit. I saw two things wrong with
> > the existing code.
> > 
> > 1. It didn't bother trying to decode the bit.
> > -  While we do not *intentionally* emit this bit today, I think it's interesting
> >    to see if we somehow ended up with the bit set. It may also be useful in the
> >    future.
> > 
> > 2. It seemed to be the wrong bit.
> > -  The docs are pretty poor wrt which bit this actually occupies. To me, it
> >    /looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
> >    verified it should be 28 by looking at the simulator code.
> 
> Yeah, it sure looks like 28 to me...I must've botched it when typing up the
> tables.
> 
> One comment below.
> 
> > I also added the most basic support for GOTO simply so we don't need to remember
> > to change the function in the future.
> > 
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_defines.h |  1 +
> >  src/mesa/drivers/dri/i965/brw_disasm.c  | 29 ++++++++++++++++++++++++++---
> >  src/mesa/drivers/dri/i965/brw_inst.h    |  2 +-
> >  3 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> > index 53cd75e..ed94bcc 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -820,6 +820,7 @@ enum opcode {
> >     BRW_OPCODE_MSAVE =	44,  /**< Pre-Gen6 */
> >     BRW_OPCODE_MRESTORE = 45, /**< Pre-Gen6 */
> >     BRW_OPCODE_PUSH =	46,  /**< Pre-Gen6 */
> > +   BRW_OPCODE_GOTO =	46,  /**< Gen8+    */
> >     BRW_OPCODE_POP =	47,  /**< Pre-Gen6 */
> >     BRW_OPCODE_WAIT =	48,
> >     BRW_OPCODE_SEND =	49,
> > diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c b/src/mesa/drivers/dri/i965/brw_disasm.c
> > index 53ec767..013058e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_disasm.c
> > +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
> > @@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
> >  }
> >  
> >  static bool
> > +has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
> > +{
> > +   if (brw->gen < 8)
> > +      return false;
> > +
> > +   return opcode == BRW_OPCODE_IF ||
> > +          opcode == BRW_OPCODE_ELSE ||
> > +          opcode == BRW_OPCODE_GOTO ||
> > +          opcode == BRW_OPCODE_ENDIF;
> > +}
> 
> I don't think ENDIF has BranchCtrl.  With that removed, this is:
> 
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Yes. Matt caught this as well. Unfortunately the search for branch_ctrl returns
nothing in bspec, so I had to manually click every instruction in the ISA (I
didn't want to assume anything). Else is next to Endif... I am pretty sure I
clicked Else twice.

Thanks. I'll consider the other comment Matt left and then either resubmit with
a change for him, or push.

-- 
Ben Widawsky, Intel Open Source Technology Center