On Mon, Feb 13, 2017 at 3:25 AM, Lonnberg, Toni <[email protected]> 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.
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
