[Mesa-dev] [PATCH 3/3] Changed shader disassembler number formatting to use integers when the "disasm" debug flag is used. Register types and regions are also now formatted more like in the architecture documentation.

Matt Turner mattst88 at gmail.com
Mon Feb 13 14:11:58 UTC 2017


On Mon, Feb 13, 2017 at 3:25 AM, Lonnberg, Toni <toni.lonnberg at intel.com> wrote:
> ---

We need a summary message that gives an overview of what's going on.

I see at least the following changes:

  - Colon before the register type
  - Semi-colon instead of comma after VertStride
  - Printing swizzles on 3-src arguments with RepCtrl set
  - Disassembling floating-point immediates as hex
  - Changing the type s/D/UD/ of message descriptors

Each of those should really be a patch on its own.

>  src/mesa/drivers/dri/i965/brw_disasm.c         | 101 +++++++++++++++----------
>  src/mesa/drivers/dri/i965/brw_eu_emit.c        |   2 +-
>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp |   2 +-
>  3 files changed, 65 insertions(+), 40 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c b/src/mesa/drivers/dri/i965/brw_disasm.c
> index 2026312..34bea08 100644
> --- a/src/mesa/drivers/dri/i965/brw_disasm.c
> +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
> @@ -237,24 +237,24 @@ static const char *const access_mode[2] = {
>  };
>
>  static const char * const reg_encoding[] = {
> -   [BRW_HW_REG_TYPE_UD]          = "UD",
> -   [BRW_HW_REG_TYPE_D]           = "D",
> -   [BRW_HW_REG_TYPE_UW]          = "UW",
> -   [BRW_HW_REG_TYPE_W]           = "W",
> -   [BRW_HW_REG_NON_IMM_TYPE_UB]  = "UB",
> -   [BRW_HW_REG_NON_IMM_TYPE_B]   = "B",
> -   [GEN7_HW_REG_NON_IMM_TYPE_DF] = "DF",
> -   [BRW_HW_REG_TYPE_F]           = "F",
> -   [GEN8_HW_REG_TYPE_UQ]         = "UQ",
> -   [GEN8_HW_REG_TYPE_Q]          = "Q",
> -   [GEN8_HW_REG_NON_IMM_TYPE_HF] = "HF",
> +   [BRW_HW_REG_TYPE_UD]          = ":UD",
> +   [BRW_HW_REG_TYPE_D]           = ":D",
> +   [BRW_HW_REG_TYPE_UW]          = ":UW",
> +   [BRW_HW_REG_TYPE_W]           = ":W",
> +   [BRW_HW_REG_NON_IMM_TYPE_UB]  = ":UB",
> +   [BRW_HW_REG_NON_IMM_TYPE_B]   = ":B",
> +   [GEN7_HW_REG_NON_IMM_TYPE_DF] = ":DF",
> +   [BRW_HW_REG_TYPE_F]           = ":F",
> +   [GEN8_HW_REG_TYPE_UQ]         = ":UQ",
> +   [GEN8_HW_REG_TYPE_Q]          = ":Q",
> +   [GEN8_HW_REG_NON_IMM_TYPE_HF] = ":HF",
>  };
>
>  static const char *const three_source_reg_encoding[] = {
> -   [BRW_3SRC_TYPE_F]  = "F",
> -   [BRW_3SRC_TYPE_D]  = "D",
> -   [BRW_3SRC_TYPE_UD] = "UD",
> -   [BRW_3SRC_TYPE_DF] = "DF",
> +   [BRW_3SRC_TYPE_F]  = ":F",
> +   [BRW_3SRC_TYPE_D]  = ":D",
> +   [BRW_3SRC_TYPE_UD] = ":UD",
> +   [BRW_3SRC_TYPE_DF] = ":DF",
>  };
>
>  static const char *const reg_file[4] = {
> @@ -839,7 +839,7 @@ src_align1_region(FILE *file,
>     int err = 0;
>     string(file, "<");
>     err |= control(file, "vert stride", vert_stride, _vert_stride, NULL);
> -   string(file, ",");
> +   string(file, ";");
>     err |= control(file, "width", width, _width, NULL);
>     string(file, ",");
>     err |= control(file, "horiz_stride", horiz_stride, _horiz_stride, NULL);
> @@ -989,11 +989,11 @@ src0_3src(FILE *file, const struct gen_device_info *devinfo, brw_inst *inst)
>     if (src0_subreg_nr || brw_inst_3src_src0_rep_ctrl(devinfo, inst))
>        format(file, ".%d", src0_subreg_nr);
>     if (brw_inst_3src_src0_rep_ctrl(devinfo, inst))
> -      string(file, "<0,1,0>");
> +      string(file, "<0;1,0>");
>     else {
> -      string(file, "<4,4,1>");
> -      err |= src_swizzle(file, brw_inst_3src_src0_swizzle(devinfo, inst));
> +      string(file, "<4;4,1>");
>     }
> +   err |= src_swizzle(file, brw_inst_3src_src0_swizzle(devinfo, inst));
>     err |= control(file, "src da16 reg type", three_source_reg_encoding,
>                    brw_inst_3src_src_type(devinfo, inst), NULL);
>     return err;
> @@ -1016,11 +1016,11 @@ src1_3src(FILE *file, const struct gen_device_info *devinfo, brw_inst *inst)
>     if (src1_subreg_nr || brw_inst_3src_src1_rep_ctrl(devinfo, inst))
>        format(file, ".%d", src1_subreg_nr);
>     if (brw_inst_3src_src1_rep_ctrl(devinfo, inst))
> -      string(file, "<0,1,0>");
> +      string(file, "<0;1,0>");
>     else {
> -      string(file, "<4,4,1>");
> -      err |= src_swizzle(file, brw_inst_3src_src1_swizzle(devinfo, inst));
> +      string(file, "<4;4,1>");
>     }
> +   err |= src_swizzle(file, brw_inst_3src_src1_swizzle(devinfo, inst));

So, this is changing it so that it prints the swizzle even in the
presence of RepCtrl. At minimum, I'd like this to be behind if
(disasm) (which itself really needs a better name).

>     err |= control(file, "src da16 reg type", three_source_reg_encoding,
>                    brw_inst_3src_src_type(devinfo, inst), NULL);
>     return err;
> @@ -1044,11 +1044,11 @@ src2_3src(FILE *file, const struct gen_device_info *devinfo, brw_inst *inst)
>     if (src2_subreg_nr || brw_inst_3src_src2_rep_ctrl(devinfo, inst))
>        format(file, ".%d", src2_subreg_nr);
>     if (brw_inst_3src_src2_rep_ctrl(devinfo, inst))
> -      string(file, "<0,1,0>");
> +      string(file, "<0;1,0>");
>     else {
> -      string(file, "<4,4,1>");
> -      err |= src_swizzle(file, brw_inst_3src_src2_swizzle(devinfo, inst));
> +      string(file, "<4;4,1>");
>     }
> +   err |= src_swizzle(file, brw_inst_3src_src2_swizzle(devinfo, inst));
>     err |= control(file, "src da16 reg type", three_source_reg_encoding,
>                    brw_inst_3src_src_type(devinfo, inst), NULL);
>     return err;
> @@ -1057,37 +1057,62 @@ src2_3src(FILE *file, const struct gen_device_info *devinfo, brw_inst *inst)
>  static int
>  imm(FILE *file, const struct gen_device_info *devinfo, unsigned type, brw_inst *inst)
>  {
> +   bool disasm = (INTEL_DEBUG & DEBUG_DISASM) != 0;
> +
>     switch (type) {
>     case BRW_HW_REG_TYPE_UD:
> -      format(file, "0x%08xUD", brw_inst_imm_ud(devinfo, inst));
> +      format(file, "0x%08x:UD", brw_inst_imm_ud(devinfo, inst));
>        break;
>     case BRW_HW_REG_TYPE_D:
> -      format(file, "%dD", brw_inst_imm_d(devinfo, inst));
> +      format(file, "%d:D", brw_inst_imm_d(devinfo, inst));
>        break;
>     case BRW_HW_REG_TYPE_UW:
> -      format(file, "0x%04xUW", (uint16_t) brw_inst_imm_ud(devinfo, inst));
> +      format(file, "0x%04x:UW", (uint16_t) brw_inst_imm_ud(devinfo, inst));
>        break;
>     case BRW_HW_REG_TYPE_W:
> -      format(file, "%dW", (int16_t) brw_inst_imm_d(devinfo, inst));
> +      format(file, "%d:W", (int16_t) brw_inst_imm_d(devinfo, inst));
>        break;
>     case BRW_HW_REG_IMM_TYPE_UV:
> -      format(file, "0x%08xUV", brw_inst_imm_ud(devinfo, inst));
> +      format(file, "0x%08x:UV", brw_inst_imm_ud(devinfo, inst));
>        break;
>     case BRW_HW_REG_IMM_TYPE_VF:
> -      format(file, "[%-gF, %-gF, %-gF, %-gF]VF",
> -             brw_vf_to_float(brw_inst_imm_ud(devinfo, inst)),
> -             brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 8),
> -             brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 16),
> -             brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 24));
> +      if (disasm) {
> +         format(file, "0x%08x:VF", brw_inst_imm_ud(devinfo, inst));
> +      } else {
> +         format(file, "[%-gF, %-gF, %-gF, %-gF]:VF",
> +                brw_vf_to_float(brw_inst_imm_ud(devinfo, inst)),
> +                brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 8),
> +                brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 16),
> +                brw_vf_to_float(brw_inst_imm_ud(devinfo, inst) >> 24));
> +      }
>        break;
>     case BRW_HW_REG_IMM_TYPE_V:
> -      format(file, "0x%08xV", brw_inst_imm_ud(devinfo, inst));
> +      format(file, "0x%08x:V", brw_inst_imm_ud(devinfo, inst));
>        break;
>     case BRW_HW_REG_TYPE_F:
> -      format(file, "%-gF", brw_inst_imm_f(devinfo, inst));
> +      if (disasm) {
> +         union {
> +            float f;
> +            unsigned u;
> +         } dt;
> +         dt.f = brw_inst_imm_f(devinfo, inst);
> +         format(file, "0x%08x:F", dt.u);
> +      } else {
> +         format(file, "%-g:F", brw_inst_imm_f(devinfo, inst));
> +      }
>        break;
>     case GEN8_HW_REG_IMM_TYPE_DF:
> -      format(file, "%-gDF", brw_inst_imm_df(devinfo, inst));
> +      if (disasm)
> +      {

Brace goes on the same line as the if

> +         union {
> +            double d;
> +            uint64_t u;
> +         } dt;
> +         dt.d = brw_inst_imm_df(devinfo, inst);
> +         format(file, "0x%016"PRIx64":DF", dt.u);
> +      } else {
> +         format(file, "%-g:DF", brw_inst_imm_df(devinfo, inst));
> +      }
>        break;
>     case GEN8_HW_REG_IMM_TYPE_HF:
>        string(file, "Half Float IMM");
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index f4bec33..c1868d6 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -624,7 +624,7 @@ brw_set_message_descriptor(struct brw_codegen *p,
>  {
>     const struct gen_device_info *devinfo = p->devinfo;
>
> -   brw_set_src1(p, inst, brw_imm_d(0));
> +   brw_set_src1(p, inst, brw_imm_ud(0));

I would rather have changes to the generated code in a separate patch.

>
>     /* For indirect sends, `inst` will not be the SEND/SENDC instruction
>      * itself; instead, it will be a MOV/OR into the address register.
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 79ff76b..a931bf4 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -480,7 +480,7 @@ fs_generator::generate_urb_write(fs_inst *inst, struct brw_reg payload)
>
>     brw_set_dest(p, insn, brw_null_reg());
>     brw_set_src0(p, insn, payload);
> -   brw_set_src1(p, insn, brw_imm_d(0));
> +   brw_set_src1(p, insn, brw_imm_ud(0));

Same.


More information about the mesa-dev mailing list