On Wednesday, November 19, 2014 09:15:32 AM Ben Widawsky wrote:
> 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 <kenn...@whitecape.org>
> > > Signed-off-by: Ben Widawsky <b...@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 <kenn...@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.Odd, search for BranchCtrl and branch_ctrl turned up things for me. Just had to consider the spellings :( > > Thanks. I'll consider the other comment Matt left and then either resubmit > with > a change for him, or push. Oh, I hadn't seen Matt's reply. I like his suggestion as well. I don't think it's worth resubmitting - if you make both of those changes, you can probably just go ahead and push the patch.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
