From f37c9c873cc048920758af41e76afc2dc619201c Mon Sep 17 00:00:00 2001 From: Caio Oliveira Date: Fri, 18 Jul 2025 09:40:23 -0700 Subject: [PATCH] brw: Fix printing of blocks in disassembly when BRW is available When disassembling and BRW IR is available (which happens in the generator), there will be pointers to the BRW's basic block structures that are used to print the block numbers and predecessor/successors in the output. There are two challenges: - Because DO and FLOW instructions are not real instructions, they are not emitted in the output but would still cause the output to contain empty blocks. Previous code accounted for DO but still had problems. - DO blocks have special physical links that don't make sense when the DO is not emitted at the end, but they would be shown even if that block was omitted. These issues can be seen here (edited to remove non-essential bits) ``` START B0 (2 cycles) mov(8) g126<1>UD 0x3f800000UD END B0 ->B1 START B2 <-B1 <-B4 (0 cycles) END B2 ->B3 START B3 <-B2 (260 cycles) LABEL1: mov(8) g1<1>D 0D cmp.ge.f0.0(8) null<1>D g2<0,1,0>D 10D sync nop(1) null<0,1,0>UB send(1) g0UD g1UD nullUD (+f0.0) break(8) JIP: LABEL0 UIP: LABEL0 END B3 ->B1 ->B5 ->B4 START B4 <-B3 (1000 cycles) sync nop(1) null<0,1,0>UB mov(8) g126<1>UD g0<0,1,0>UD LABEL0: while(8) JIP: LABEL1 END B4 ->B2 START B5 <-B1 <-B3 (20 cycles) ``` For example: - Block 1 is missing (a skipped DO block) - Block 2 is empty (it was a FLOW block) - Block 3 ends with a link to Block 1 (the special links involving DO blocks). Two key changes were made to fix this. First, skip the DO and FLOW blocks completely. The use_tail ensures that the instruction group is reused to avoid empty blocks. Second, when printing, the successors and predecessors, walk through the skipped blocks. And finally, don't print the special blocks. With the fix, here's the output. Note the blocks retain their original BRW IR number. ``` START B0 (2 cycles) mov(8) g127<1>UD 0x3f800000UD END B0 ->B3 START B3 <-B0 <-B4 (260 cycles) LABEL1: mov(8) g1<1>D 0D cmp.ge.f0.0(8) null<1>D g2<0,1,0>D 10D sync nop(1) null<0,1,0>UB send(1) g0UD g1UD nullUD (+f0.0) break(8) JIP: LABEL0 UIP: LABEL0 END B3 ->B5 ->B4 START B4 <-B3 (1000 cycles) sync nop(1) null<0,1,0>UB mov(8) g127<1>UD g0<0,1,0>UD LABEL0: while(8) JIP: LABEL1 END B4 ->B3 START B5 <-B3 (20 cycles) ``` Issue was spotted by Ken. Fixes: d2c39b17793 ("intel/brw: Always have a (non-DO) block after a DO in the CFG") Reviewed-by: Kenneth Graunke Part-of: --- src/intel/compiler/brw_disasm_info.cpp | 79 +++++++++++++++++++------- 1 file changed, 57 insertions(+), 22 deletions(-) diff --git a/src/intel/compiler/brw_disasm_info.cpp b/src/intel/compiler/brw_disasm_info.cpp index a703f63e922..4fec8307671 100644 --- a/src/intel/compiler/brw_disasm_info.cpp +++ b/src/intel/compiler/brw_disasm_info.cpp @@ -28,6 +28,54 @@ #include "dev/intel_debug.h" #include "compiler/nir/nir.h" +static bool +is_do_block(struct bblock_t *block) +{ + return block->start()->opcode == BRW_OPCODE_DO; +} + +static bool +is_flow_block(struct bblock_t *block) +{ + return block->start()->opcode == SHADER_OPCODE_FLOW; +} + +static bool +should_omit_link(struct bblock_t *block, + struct bblock_link *link) +{ + return link->kind == bblock_link_physical && + (is_do_block(block) || is_do_block(link->block)); +} + +static void +print_successors_for_disasm(struct bblock_t *block) +{ + brw_foreach_list_typed(struct bblock_link, succ, link, + &block->children) { + if (should_omit_link(block, succ)) + continue; + if (is_do_block(succ->block) || is_flow_block(succ->block)) + print_successors_for_disasm(succ->block); + else + fprintf(stderr, " ->B%d", succ->block->num); + } +} + +static void +print_predecessors_for_disasm(struct bblock_t *block) +{ + brw_foreach_list_typed(struct bblock_link, pred, link, + &block->parents) { + if (should_omit_link(block, pred)) + continue; + if (is_do_block(pred->block) || is_flow_block(pred->block)) + print_predecessors_for_disasm(pred->block); + else + fprintf(stderr, " <-B%d", pred->block->num); + } +} + void dump_assembly(void *assembly, int start_offset, int end_offset, struct disasm_info *disasm, const unsigned *block_latency) @@ -52,11 +100,7 @@ dump_assembly(void *assembly, int start_offset, int end_offset, if (group->block_start) { fprintf(stderr, " START B%d", group->block_start->num); - brw_foreach_list_typed(struct bblock_link, predecessor_link, link, - &group->block_start->parents) { - struct bblock_t *predecessor_block = predecessor_link->block; - fprintf(stderr, " <-B%d", predecessor_block->num); - } + print_predecessors_for_disasm(group->block_start); if (block_latency) fprintf(stderr, " (%u cycles)", block_latency[group->block_start->num]); @@ -78,11 +122,7 @@ dump_assembly(void *assembly, int start_offset, int end_offset, if (group->block_end) { fprintf(stderr, " END B%d", group->block_end->num); - brw_foreach_list_typed(struct bblock_link, successor_link, link, - &group->block_end->children) { - struct bblock_t *successor_block = successor_link->block; - fprintf(stderr, " ->B%d", successor_block->num); - } + print_successors_for_disasm(group->block_end); fprintf(stderr, "\n"); } } @@ -135,20 +175,15 @@ disasm_annotate(struct disasm_info *disasm, } #endif - if (cfg->blocks[disasm->cur_block]->start() == inst) { - group->block_start = cfg->blocks[disasm->cur_block]; + if (inst->opcode == BRW_OPCODE_DO || + inst->opcode == SHADER_OPCODE_FLOW) { + disasm->use_tail = true; + disasm->cur_block++; + return; } - /* There is no hardware DO instruction on Gfx6+, so since DO always - * starts a basic block, we need to set the .block_start of the next - * instruction's annotation with a pointer to the bblock started by - * the DO. - * - * There's also only complication from emitting an annotation without - * a corresponding hardware instruction to disassemble. - */ - if (inst->opcode == BRW_OPCODE_DO) { - disasm->use_tail = true; + if (cfg->blocks[disasm->cur_block]->start() == inst) { + group->block_start = cfg->blocks[disasm->cur_block]; } if (cfg->blocks[disasm->cur_block]->end() == inst) {