From 695f5314d61410208cb52cde0fe3e25a7f8f0cf2 Mon Sep 17 00:00:00 2001 From: Caio Oliveira Date: Fri, 23 Aug 2024 10:46:13 -0700 Subject: [PATCH] intel/brw: Simplify fs_inst annotation When INTEL_DEBUG=ann is also set, the disassembler would annotate the output with either a string or the string verison of a NIR instruction. This was done by keeping two pointers (but only using one at a time). Change the code to print the instruction into a string instead of keeping it pointer around (peg the string to the shader). That way, only one pointer is needed for annotations. Because that serialization is not free, only do that when the environment variable is set. Since we are here, move the annotation string field to the end, moving it to the least commonly used cacheline. Further packing might allow the entire fs_inst to fit in two cachelines. For release builds, don't even add the debug annotation to the struct. Reviewed-by: Kenneth Graunke Part-of: --- src/intel/compiler/brw_disasm_info.cpp | 13 ++----------- src/intel/compiler/brw_disasm_info.h | 3 +-- src/intel/compiler/brw_fs_builder.h | 12 +++++++----- src/intel/compiler/brw_fs_lower.cpp | 2 +- src/intel/compiler/brw_fs_nir.cpp | 17 ++++++++++++++--- src/intel/compiler/brw_ir_fs.h | 15 ++++++++------- 6 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/intel/compiler/brw_disasm_info.cpp b/src/intel/compiler/brw_disasm_info.cpp index ca4588683be..ce83840159a 100644 --- a/src/intel/compiler/brw_disasm_info.cpp +++ b/src/intel/compiler/brw_disasm_info.cpp @@ -34,7 +34,6 @@ dump_assembly(void *assembly, int start_offset, int end_offset, { const struct brw_isa_info *isa = disasm->isa; const char *last_annotation_string = NULL; - const void *last_annotation_ir = NULL; void *mem_ctx = ralloc_context(NULL); const struct brw_label *root_label = @@ -64,15 +63,6 @@ dump_assembly(void *assembly, int start_offset, int end_offset, fprintf(stderr, "\n"); } - if (last_annotation_ir != group->ir) { - last_annotation_ir = group->ir; - if (last_annotation_ir) { - fprintf(stderr, " "); - nir_print_instr((nir_instr *)group->ir, stderr); - fprintf(stderr, "\n"); - } - } - if (last_annotation_string != group->annotation) { last_annotation_string = group->annotation; if (last_annotation_string) @@ -139,10 +129,11 @@ disasm_annotate(struct disasm_info *disasm, exec_list_get_tail_raw(&disasm->group_list), link); } +#ifndef NDEBUG if (INTEL_DEBUG(DEBUG_ANNOTATION)) { - group->ir = inst->ir; group->annotation = inst->annotation; } +#endif if (bblock_start(cfg->blocks[disasm->cur_block]) == inst) { group->block_start = cfg->blocks[disasm->cur_block]; diff --git a/src/intel/compiler/brw_disasm_info.h b/src/intel/compiler/brw_disasm_info.h index 1779c05a335..1010a44cc78 100644 --- a/src/intel/compiler/brw_disasm_info.h +++ b/src/intel/compiler/brw_disasm_info.h @@ -48,8 +48,7 @@ struct inst_group { struct bblock_t *block_start; struct bblock_t *block_end; - /* Annotation for the generated IR. One of the two can be set. */ - const void *ir; + /* Annotation for the generated IR. */ const char *annotation; }; diff --git a/src/intel/compiler/brw_fs_builder.h b/src/intel/compiler/brw_fs_builder.h index f736fc73028..691840991bd 100644 --- a/src/intel/compiler/brw_fs_builder.h +++ b/src/intel/compiler/brw_fs_builder.h @@ -63,8 +63,11 @@ namespace brw { _group(inst->group), force_writemask_all(inst->force_writemask_all) { +#ifndef NDEBUG annotation.str = inst->annotation; - annotation.ir = inst->ir; +#else + annotation.str = NULL; +#endif } /** @@ -152,11 +155,10 @@ namespace brw { * Construct a builder with the given debug annotation info. */ fs_builder - annotate(const char *str, const void *ir = NULL) const + annotate(const char *str) const { fs_builder bld = *this; bld.annotation.str = str; - bld.annotation.ir = ir; return bld; } @@ -334,8 +336,9 @@ namespace brw { inst->group = _group; inst->force_writemask_all = force_writemask_all; +#ifndef NDEBUG inst->annotation = annotation.str; - inst->ir = annotation.ir; +#endif if (block) static_cast(cursor)->insert_before(block, inst); @@ -966,7 +969,6 @@ namespace brw { /** Debug annotation info. */ struct { const char *str; - const void *ir; } annotation; }; } diff --git a/src/intel/compiler/brw_fs_lower.cpp b/src/intel/compiler/brw_fs_lower.cpp index 46236a32562..9bc8adef646 100644 --- a/src/intel/compiler/brw_fs_lower.cpp +++ b/src/intel/compiler/brw_fs_lower.cpp @@ -796,7 +796,7 @@ brw_fs_lower_load_subgroup_invocation(fs_visitor &s) continue; const fs_builder abld = - fs_builder(&s, block, inst).annotate("SubgroupInvocation", NULL); + fs_builder(&s, block, inst).annotate("SubgroupInvocation"); const fs_builder ubld8 = abld.group(8, 0).exec_all(); ubld8.UNDEF(inst->dst); diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp index 7a4a25e16b5..ef57d5850ca 100644 --- a/src/intel/compiler/brw_fs_nir.cpp +++ b/src/intel/compiler/brw_fs_nir.cpp @@ -28,6 +28,7 @@ #include "nir.h" #include "nir_intrinsics.h" #include "nir_search_helpers.h" +#include "dev/intel_debug.h" #include "util/u_math.h" #include "util/bitscan.h" @@ -59,6 +60,8 @@ struct nir_to_brw_state { struct brw_fs_bind_info *ssa_bind_infos; brw_reg *uniform_values; brw_reg *system_values; + + bool annotate; }; static brw_reg get_nir_src(nir_to_brw_state &ntb, const nir_src &src); @@ -301,7 +304,7 @@ emit_system_values_block(nir_to_brw_state &ntb, nir_block *block) reg = &ntb.system_values[SYSTEM_VALUE_HELPER_INVOCATION]; if (reg->file == BAD_FILE) { const fs_builder abld = - ntb.bld.annotate("gl_HelperInvocation", NULL); + ntb.bld.annotate("gl_HelperInvocation"); /* On Gfx6+ (gl_HelperInvocation is only exposed on Gfx7+) the * pixel mask is in g1.7 of the thread payload. @@ -2464,7 +2467,7 @@ set_gs_stream_control_data_bits(nir_to_brw_state &ntb, const brw_reg &vertex_cou if (stream_id == 0) return; - const fs_builder abld = ntb.bld.annotate("set stream control data bits", NULL); + const fs_builder abld = ntb.bld.annotate("set stream control data bits"); /* reg::sid = stream_id */ brw_reg sid = abld.MOV(brw_imm_ud(stream_id)); @@ -8725,7 +8728,12 @@ shuffle_from_32bit_read(const fs_builder &bld, static void fs_nir_emit_instr(nir_to_brw_state &ntb, nir_instr *instr) { - ntb.bld = ntb.bld.annotate(NULL, instr); +#ifndef NDEBUG + if (unlikely(ntb.annotate)) { + /* Use shader mem_ctx since annotations outlive the NIR conversion. */ + ntb.bld = ntb.bld.annotate(nir_instr_as_str(instr, ntb.s.mem_ctx)); + } +#endif switch (instr->type) { case nir_instr_type_alu: @@ -8913,6 +8921,9 @@ nir_to_brw(fs_visitor *s) .bld = fs_builder(s).at_end(), }; + if (INTEL_DEBUG(DEBUG_ANNOTATION)) + ntb.annotate = true; + if (ENABLE_FS_TEST_DISPATCH_PACKING) brw_fs_test_dispatch_packing(ntb.bld); diff --git a/src/intel/compiler/brw_ir_fs.h b/src/intel/compiler/brw_ir_fs.h index 2e134e662fa..ebb04ad5794 100644 --- a/src/intel/compiler/brw_ir_fs.h +++ b/src/intel/compiler/brw_ir_fs.h @@ -122,13 +122,6 @@ public: */ bool has_sampler_residency() const; - /** @{ - * Annotation for the generated IR. One of the two can be set. - */ - const void *ir; - const char *annotation; - /** @} */ - uint8_t sources; /**< Number of brw_reg sources. */ /** @@ -225,6 +218,14 @@ public: brw_reg dst; brw_reg *src; brw_reg builtin_src[4]; + +#ifndef NDEBUG + /** @{ + * Annotation for the generated IR. + */ + const char *annotation; + /** @} */ +#endif }; /**