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

[Matt Turner]

On Tue, Nov 18, 2014 at 12:35 PM, Ben Widawsky
<benjamin.widawsky at intel.com> 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.

Indeed, if you search for "branch control" in the BSpec, you'll find a
page describing "AccWrCtrl/BranchCtrl" and AccWrCtrl is definitely bit
28.

>
> 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 see that ENDIF has branch_ctrl.

> +}
> +
> +static bool
>  is_logic_instruction(unsigned opcode)
>  {
>     return opcode == BRW_OPCODE_AND ||
> @@ -217,6 +229,11 @@ static const char *const accwr[2] = {
>     [1] = "AccWrEnable"
>  };
>
> +static const char *const branch_ctrl[2] = {
> +   [0] = "",
> +   [1] = "BranchCtrl"
> +};
> +
>  static const char *const wectrl[2] = {
>     [0] = "",
>     [1] = "WE_all"
> @@ -1544,9 +1561,15 @@ brw_disassemble_inst(FILE *file, struct brw_context *brw, brw_inst *inst,
>        err |= control(file, "compaction", cmpt_ctrl, is_compacted, &space);
>        err |= control(file, "thread control", thread_ctrl,
>                       brw_inst_thread_control(brw, inst), &space);
> -      if (brw->gen >= 6)
> -         err |= control(file, "acc write control", accwr,
> -                        brw_inst_acc_wr_control(brw, inst), &space);
> +      if (brw->gen >= 6) {
> +         if (has_branch_ctrl(brw, opcode)) {
> +            err |= control(file, "branch ctrl", branch_ctrl,
> +                           brw_inst_branch_control(brw, inst), &space);
> +         } else {
> +            err |= control(file, "acc write control", accwr,
> +                           brw_inst_acc_wr_control(brw, inst), &space);
> +         }

I was a little confused by this block until I saw the gen < 8 check in
has_branch_ctrl(). I'd probably change the block to

   if (has_branch_ctrl(...)) {
      ...
   } else if (brw->gen >= 6) {
      ...
   }

I don't have a strong preference whether the gen 8 check happens
inside or outside the has_branch_ctrl() function.

Everything else looks good.

Reviewed-by: Matt Turner <mattst88 at gmail.com>