From 7fcf65d2c2cc65dcfb0de9b2b075a513cb698c51 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 24 Aug 2026 22:13:57 +0100 Subject: [PATCH] asm: General clean up of the CFG code and remove redundant calculations --- src/check_asm.cpp | 114 ++++++++++++---------------------- src/check_asm_cfg.cpp | 141 +++++++++++++++++++++++++++--------------- 2 files changed, 132 insertions(+), 123 deletions(-) diff --git a/src/check_asm.cpp b/src/check_asm.cpp index e57839569..2b0fde8d7 100644 --- a/src/check_asm.cpp +++ b/src/check_asm.cpp @@ -1110,7 +1110,7 @@ template gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tmpl_entity, AstAsmInstruction *instr, u16 mnemonic, u16 pseudo_mnemonic, Slice const &operands, u8 previous_prefix, Ast *previous_prefix_instr, - AsmMnemonicAccumulator *asm_acc) { + AsmCfg *cfg) { GB_ASSERT(mnemonic > 0); auto forms = asm_ctx->encoding_forms(mnemonic); auto clobber_forms = asm_ctx->clobber_forms(mnemonic); @@ -1140,7 +1140,7 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm break; } - map_set(&asm_acc->instruction_facts, instr, facts); + map_set(&cfg->instruction_facts, instr, facts); }); @@ -1578,14 +1578,12 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm // through a parameter pointer does NOT require stack realignment, so // implies_clobber_memory() is intentionally NOT used here. if (clobber.is_call_or_mem()) { - asm_acc->saw_call_or_mem = true; + cfg->saw_call_or_mem = true; } u16 pinned_mask = 0; - for (auto const &ed : tmpl_entity->AsmTemplate.decls) { - if (ed.pin.len != 0) { - pinned_mask |= asm_ctx->clobber_bit_for_reg_name(ed.pin); - } + for_array(i, tmpl_entity->AsmTemplate.decls) { + pinned_mask |= asm_decl_resolve_pin_bit(asm_ctx, tmpl_entity->AsmTemplate.decls, cast(i32)i); } u16 produced = cast(u16)clobber.implicit_wr & asm_ctx->CLOBBER_REGS_NAMED; @@ -1593,50 +1591,25 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm u16 written_ops = cast(u16)clobber.written; u16 pinned_param_writes = 0; + auto const &decls = tmpl_entity->AsmTemplate.decls; for_array(i, operands) { int tslot = user_operand_target_index(cast(int)i); if (tslot < 0 || tslot >= 4 || (written_ops & (1u << tslot)) == 0) { continue; } - auto const &op = operands[i]; - - Ast *e = op.expr; + Ast *e = operands[i].expr; if (e != nullptr && e->kind == Ast_AsmRegister) { u16 b = asm_ctx->clobber_bit_for_reg_name(e->AsmRegister.name.string); produced |= b; explicit_writes |= b; continue; } - - // NOTE(bill): A write through a pinned parameter (or a width-view of one) - // defines that parameter's physical register for the read-before-write check only - auto written_pinned_reg_bit = [&](Operand const &op) -> u16 { - Entity *pe = entity_of_node(op.expr); - if (pe == nullptr || pe->kind != Entity_Variable) { - return 0; - } - auto const &decls = tmpl_entity->AsmTemplate.decls; - for_array(di, decls) { - auto const &ed = decls[di]; - if (ed.entity != pe) { - continue; - } - if (ed.pin.len != 0) { - return asm_ctx->clobber_bit_for_reg_name(ed.pin); - } - // NOTE(bill): A width-view carries no pin of its own and thus it aliases its source's register. - if (ed.view_of >= 0 && ed.view_of < cast(i32)decls.count) { - String src_pin = decls[ed.view_of].pin; - if (src_pin.len != 0) { - return asm_ctx->clobber_bit_for_reg_name(src_pin); - } - } - return 0; - } - return 0; - }; - - pinned_param_writes |= written_pinned_reg_bit(operands[i]); + Entity *pe = entity_of_node(operands[i].expr); + if (pe != nullptr && pe->kind == Entity_Variable) { + i32 di = -1; + check_asm_find_group(pe, decls, &di); // reuse existing index finder + pinned_param_writes |= asm_decl_resolve_pin_bit(asm_ctx, decls, di); + } } if (is_pseudo && @@ -1704,15 +1677,15 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm // redundant-#clobber hint. Union across the template; pinned regs excluded // so a legitimate output pin is never called "redundant". u16 implicit_wr = cast(u16)clobber.implicit_wr & asm_ctx->CLOBBER_REGS_NAMED; - asm_acc->implicit_clobbered_regs |= implicit_wr & ~pinned_mask; + cfg->implicit_clobbered_regs |= implicit_wr & ~pinned_mask; // Approximate staleness. An output that was explicitly produced (literal %reg write) // and is later implicitly clobbered — without this same instruction re-producing it — // is marked stale. Explicit re-production clears it. Implicitly-produced outputs // (RDTSC->RDX) are never tracked, so they never false-fire. - asm_acc->explicitly_produced_regs |= explicit_writes; - asm_acc->stale_outputs &= ~explicit_writes; - asm_acc->stale_outputs |= implicit_wr & asm_acc->explicitly_produced_regs & ~explicit_writes; + cfg->explicitly_produced_regs |= explicit_writes; + cfg->stale_outputs &= ~explicit_writes; + cfg->stale_outputs |= implicit_wr & cfg->explicitly_produced_regs & ~explicit_writes; } { @@ -1734,7 +1707,7 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm asm_ctx->clobber_implicit_regs(&tmpl_entity->AsmTemplate.clobber_registers_set, produced); // Purity inference - if (asm_acc->can_be_pure) { + if (cfg->can_be_pure) { // NOTE(bill): Only the first violating instruction is recorded // The later ones don't overwrite the reason. char const *why = nullptr; @@ -1760,9 +1733,9 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm } if (why != nullptr) { - asm_acc->can_be_pure = false; - asm_acc->impure_reason = why; - asm_acc->impure_reason_node = instr->name; + cfg->can_be_pure = false; + cfg->impure_reason = why; + cfg->impure_reason_node = instr->name; } } return; @@ -2416,11 +2389,16 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity } } - AsmMnemonicAccumulator asm_acc = {}; - map_init(&asm_acc.instruction_facts); - defer (map_destroy(&asm_acc.instruction_facts)); + // NOTE(bill, 2026-08-24): Construct a control-flow graph (CFG) from the instructions + // to do further analysis which is not possible with an conservative straight-line approximation + // Using a CFG is a much sounder approach for calculating: + // * reads before writes + // * divergence + // * unreachable code - asm_acc.can_be_pure = true; + AsmCfg cfg = {}; + asm_cfg_init(&cfg); + defer (asm_cfg_destroy(&cfg)); // collect label decls for (Ast *instruction_ : at->instructions) { @@ -2484,9 +2462,9 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity instr->suffix_flags = suffix_flags; check_mnemonic(asm_ctx, ctx, entity, instr, mnemonic, 0, slice_from_array(operands), previous_prefix, previous_prefix_instr, - &asm_acc); + &cfg); - asm_acc.saw_any_instructions = true; + cfg.saw_any_instructions = true; previous_prefix = 0; previous_prefix_instr = nullptr; @@ -2498,9 +2476,9 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity u16 target_mnemonic = cast(u16)alias.target; check_mnemonic(asm_ctx, ctx, entity, instr, target_mnemonic, pseudo_mnemonic, slice_from_array(operands), previous_prefix, previous_prefix_instr, - &asm_acc); + &cfg); - asm_acc.saw_any_instructions = true; + cfg.saw_any_instructions = true; previous_prefix = 0; previous_prefix_instr = nullptr; @@ -2508,7 +2486,7 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity instr->suffix_flags = suffix_flags; check_pseudo_macro_mnemonic(asm_ctx, entity, instr, slice_from_array(operands)); - asm_acc.saw_any_instructions = true; + cfg.saw_any_instructions = true; previous_prefix = 0; previous_prefix_instr = nullptr; @@ -2618,18 +2596,8 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity error(previous_prefix_instr, "A prefix must be immediately followed by an instruction, but the template ended"); } - // NOTE(bill, 2026-08-24): Construct a control-flow graph (CFG) from the instructions - // to do further analysis which is not possible with an conservative straight-line approximation - // Using a CFG is a much sounder approach for calculating: - // * reads before writes - // * divergence - // * unreachable code - - AsmCfg cfg = {}; - defer (asm_cfg_destroy(&cfg)); - check_asm_cfg_build(d->init_expr, &asm_acc, &cfg); - check_asm_cfg_analyse(asm_ctx, ctx, entity, &cfg, &asm_acc); - + check_asm_cfg_build(asm_ctx, &cfg, d->init_expr, entity); + check_asm_cfg_analyse(asm_ctx, &cfg, ctx, entity); bool vet_unused = false; { @@ -2710,7 +2678,7 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity "Please add #volatile if the effect is intended."); } - if (entity->AsmTemplate.is_align_stack && !asm_acc.saw_call_or_mem) { + if (entity->AsmTemplate.is_align_stack && !cfg.saw_call_or_mem) { warning(entity->token, "#align_stack is redundant; this template makes no call and touches no memory " "that would require the stack to be realigned"); @@ -2720,12 +2688,12 @@ gb_internal void check_asm_template(AsmCtx *asm_ctx, CheckerContext *ctx, Entity bool declared_effects = entity->AsmTemplate.is_volatile || entity->AsmTemplate.clobber_memory || entity->AsmTemplate.has_observable_side_effect; - bool is_pure = asm_acc.can_be_pure && !declared_effects && !type->Proc.diverging; + bool is_pure = cfg.can_be_pure && !declared_effects && !type->Proc.diverging; entity->AsmTemplate.is_pure = is_pure; if (is_pure_annotated && !is_pure) { - Ast *node = asm_acc.impure_reason_node; - char const *why = asm_acc.impure_reason; + Ast *node = cfg.impure_reason_node; + char const *why = cfg.impure_reason; if (why == nullptr) { if (type->Proc.diverging) { why = "it is declared diverging (-> !) and computes no outputs"; diff --git a/src/check_asm_cfg.cpp b/src/check_asm_cfg.cpp index a02994ca6..8d4e4eb39 100644 --- a/src/check_asm_cfg.cpp +++ b/src/check_asm_cfg.cpp @@ -26,8 +26,7 @@ struct AsmInstructionFacts { i32 block_id; }; - -struct AsmMnemonicAccumulator { +struct AsmCfg { // Union of registers implicitly clobbered by matched forms (for redundant-#clobber hints). u16 implicit_clobbered_regs; u16 explicitly_produced_regs; @@ -46,14 +45,24 @@ struct AsmMnemonicAccumulator { Ast * impure_reason_node; PtrMap instruction_facts; -}; -struct AsmCfg { Array insts; // program-order (only for fact-carrying instrs) Array blocks; PtrMap label_block; // key: Entity_Label* + + PtrMap entity_to_index; + Array decl_pin_bit; + u64 universe_pm; }; +gb_internal void asm_cfg_init(AsmCfg *cfg) { + map_init(&cfg->instruction_facts); + map_init(&cfg->entity_to_index); + cfg->decl_pin_bit.allocator = heap_allocator(); + cfg->can_be_pure = true; +}; + + gb_internal void asm_cfg_destroy(AsmCfg *cfg) { for (auto &block : cfg->blocks) { array_free(&block.succs); @@ -63,18 +72,62 @@ gb_internal void asm_cfg_destroy(AsmCfg *cfg) { array_free(&cfg->blocks); array_free(&cfg->insts); map_destroy(&cfg->label_block); + map_destroy(&cfg->instruction_facts); + map_destroy(&cfg->entity_to_index); + array_free(&cfg->decl_pin_bit); } -gb_internal void check_asm_cfg_build(Ast *at_node, AsmMnemonicAccumulator *acc, AsmCfg *cfg) { +// The physical-register bit a decl is pinned to. A width-view carries no pin of +// its own; it inherits its source decl's pin. Returns 0 for unpinned decls. +template +gb_internal u16 asm_decl_resolve_pin_bit(AsmCtx *asm_ctx, Array const &decls, i32 di) { + if (di < 0 || di >= cast(i32)decls.count) { + return 0; + } + auto const &ed = decls[di]; + if (ed.pin.len != 0) { + return asm_ctx->clobber_bit_for_reg_name(ed.pin); + } + if (ed.view_of >= 0 && ed.view_of < cast(i32)decls.count) { + String src_pin = decls[ed.view_of].pin; + if (src_pin.len != 0) { + return asm_ctx->clobber_bit_for_reg_name(src_pin); + } + } + return 0; +} + +template +gb_internal void asm_cfg_populate_decls(AsmCtx *asm_ctx, AsmCfg *cfg, Entity *entity) { + auto const &decls = entity->AsmTemplate.decls; + cfg->universe_pm = 0; + if (decls.count > 64) { + // NOTE(bill): check_asm_cfg_analyse will err on this since this is exceed the maximum number of declarations + return; + } + array_resize(&cfg->decl_pin_bit, decls.count); + for_array(i, decls) { + Entity *e = decls[i].entity; + cfg->decl_pin_bit[i] = asm_decl_resolve_pin_bit(asm_ctx, decls, cast(i32)i); + if (e != nullptr) { + map_set(&cfg->entity_to_index, e, cast(i32)i); + cfg->universe_pm |= (cast(u64)1 << i); + } + } +} + +template +gb_internal void check_asm_cfg_build(AsmCtx *asm_ctx, AsmCfg *cfg, Ast *at_node, Entity *entity) { ast_node(at, AsmTemplate, at_node); + asm_cfg_populate_decls(asm_ctx, cfg, entity); + cfg->insts.allocator = heap_allocator(); cfg->blocks.allocator = heap_allocator(); map_init(&cfg->label_block); bool need_leader = true; - // Build basic blocks over the template body. A leader is: the first instruction, any // instruction preceded by a label, and any instruction following a control transfer. for (Ast *node : at->instructions) { @@ -93,7 +146,7 @@ gb_internal void check_asm_cfg_build(Ast *at_node, AsmMnemonicAccumulator *acc, } AstAsmInstruction *instr = &node->AsmInstruction; - AsmInstructionFacts *facts = map_get(&acc->instruction_facts, instr); + AsmInstructionFacts *facts = map_get(&cfg->instruction_facts, instr); // Prefixes and pseudo-macro ops (li/la) carry no facts and never branch. if (need_leader || cfg->blocks.count == 0) { @@ -122,7 +175,7 @@ gb_internal void check_asm_cfg_build(Ast *at_node, AsmMnemonicAccumulator *acc, AsmBlock *b = &cfg->blocks[bi]; AstAsmInstruction *last = cfg->insts[b->last]; - AsmInstructionFacts *lf = map_get(&acc->instruction_facts, last); + AsmInstructionFacts *lf = map_get(&cfg->instruction_facts, last); i32 branch_succ = -1; bool fallthrough = true; @@ -172,10 +225,10 @@ gb_internal void check_asm_cfg_build(Ast *at_node, AsmMnemonicAccumulator *acc, } } -gb_internal bool check_asm_cfg_block_leaves(AsmCfg *cfg, AsmMnemonicAccumulator *acc, i32 bi) { +gb_internal bool check_asm_cfg_block_leaves(AsmCfg *cfg, i32 bi) { AsmBlock const *b = &cfg->blocks[bi]; AstAsmInstruction *last = cfg->insts[b->last]; - AsmInstructionFacts *lf = map_get(&acc->instruction_facts, last); + AsmInstructionFacts *lf = map_get(&cfg->instruction_facts, last); if (lf != nullptr && lf->branch_target != nullptr) { i32 *t = map_get(&cfg->label_block, lf->branch_target); @@ -184,23 +237,23 @@ gb_internal bool check_asm_cfg_block_leaves(AsmCfg *cfg, AsmMnemonicAccumulator } } bool terminal = (lf != nullptr) && lf->is_terminal; - if (!terminal && bi > cast(i32)cfg->blocks.count) { + if (!terminal && (bi+1 >= cast(i32)cfg->blocks.count)) { return true; // straight-line / conditional tail with nothing after it } return false; } template -gb_internal void check_asm_cfg_report_undef_reg(AsmCtx *asm_ctx, Entity *tmpl_entity, +gb_internal void check_asm_cfg_report_undef_reg(AsmCtx *asm_ctx, AsmCfg *cfg, Entity *tmpl_entity, AstAsmInstruction *instr, String name, u16 bit) { char const *rname = asm_ctx->clobber_reg_bit_name(bit); String owner = {}; char const *role = nullptr; - for (auto const &ed : tmpl_entity->AsmTemplate.decls) { - if (ed.pin.len == 0 || ed.entity == nullptr) { - continue; - } - if (asm_ctx->clobber_bit_for_reg_name(ed.pin) != bit) { + + auto const &decls = tmpl_entity->AsmTemplate.decls; + for_array(i, decls) { + auto const &ed = decls[i]; + if (ed.entity == nullptr || cfg->decl_pin_bit[i] != bit) { continue; } if (ed.param_group == AsmTemplateEntityDeclParamGroup_Output && ed.tie < 0) { @@ -228,15 +281,14 @@ gb_internal void check_asm_cfg_report_undef_reg(AsmCtx *asm_ctx, Entity *tmpl_en } template -gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *entity, AsmCfg *cfg, - AsmMnemonicAccumulator *acc) { +gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, AsmCfg *cfg, CheckerContext *ctx, Entity *entity) { GB_ASSERT(entity->kind == Entity_AsmTemplate); auto const &decls = entity->AsmTemplate.decls; bool diverging = entity->type->Proc.diverging; if (cfg->blocks.count == 0) { // With an empty body, the CFG cannot really do nothing - if (diverging && !acc->saw_any_instructions) { + if (diverging && !cfg->saw_any_instructions) { error(entity->token, "This asm template is declared as diverging (-> !) but its body is empty and cannot diverge"); } return; @@ -248,40 +300,28 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent } u16 const REG_TOP = asm_ctx->CLOBBER_REGS_NAMED; - PtrMap entity_to_index = {}; - map_init(&entity_to_index); - defer (map_destroy(&entity_to_index)); - u64 universe_pm = 0; - for_array(i, decls) { - if (decls[i].entity != nullptr) { - map_set(&entity_to_index, decls[i].entity, cast(i32)i); - universe_pm |= (cast(u64)1 << i); - } - } + u64 const universe_pm = cfg->universe_pm; auto bit_of = [&](Entity *e) -> u64 { - i32 *ix = map_get(&entity_to_index, e); + i32 *ix = map_get(&cfg->entity_to_index, e); return ix ? (cast(u64)1 << *ix) : cast(u64)0; }; // NOTE(bill): entry seed intiailization which mirrors the linear seeding of defined_regs u16 seed_regs = 0; u64 seed_pm = 0; - for (auto const &ed : decls) { + for_array(i, decls) { + auto const &ed = decls[i]; + u16 pin_bit = cfg->decl_pin_bit[i]; if (ed.no_init) { seed_pm |= bit_of(ed.entity); - if (ed.pin.len != 0) { - seed_regs |= asm_ctx->clobber_bit_for_reg_name(ed.pin); - } + seed_regs |= pin_bit; } switch (ed.param_group) { case AsmTemplateEntityDeclParamGroup_Input: seed_pm |= bit_of(ed.entity); - if (ed.pin.len != 0) { - seed_regs |= asm_ctx->clobber_bit_for_reg_name(ed.pin); - } + seed_regs |= pin_bit; break; case AsmTemplateEntityDeclParamGroup_Output: - // NOTE(bill): input provides the value if (ed.tie >= 0) { seed_pm |= bit_of(ed.entity); } @@ -327,7 +367,7 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent u64 gp = 0; AsmBlock const &b = cfg->blocks[bi]; for (i32 ii = b.first; ii <= b.last; ii++) { - AsmInstructionFacts *f = map_get(&acc->instruction_facts, cfg->insts[ii]); + AsmInstructionFacts *f = map_get(&cfg->instruction_facts, cfg->insts[ii]); if (f == nullptr) { continue; } @@ -444,7 +484,7 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent for (i32 ii = b.first; ii <= b.last; ii++) { AstAsmInstruction *instr = cfg->insts[ii]; - AsmInstructionFacts *f = map_get(&acc->instruction_facts, instr); + AsmInstructionFacts *f = map_get(&cfg->instruction_facts, instr); if (f == nullptr) { continue; } @@ -454,12 +494,12 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent if ((undef & bit) == 0) { continue; } - check_asm_cfg_report_undef_reg(asm_ctx, entity, instr, f->name, bit); + check_asm_cfg_report_undef_reg(asm_ctx, cfg, entity, instr, f->name, bit); reported_regs |= bit; } for (Entity *pe : f->read_params) { - i32 *ix = map_get(&entity_to_index, pe); + i32 *ix = map_get(&cfg->entity_to_index, pe); if (ix == nullptr) { continue; } @@ -489,7 +529,7 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent if (!cfg->blocks[bi].reachable) { continue; } - if (!check_asm_cfg_block_leaves(cfg, acc, cast(i32)bi)) { + if (!check_asm_cfg_block_leaves(cfg, cast(i32)bi)) { continue; } any_exit = true; @@ -499,7 +539,8 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent // NOTE(bill): Outputs must be assigned on every path that returns if (any_exit && !diverging) { - for (auto const &ed : decls) { + for_array(i, decls) { + auto const &ed = decls[i]; if (ed.param_group != AsmTemplateEntityDeclParamGroup_Output) { continue; } @@ -507,10 +548,10 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent continue; } - bool written; - if (ed.pin.len != 0) { - u16 bit = asm_ctx->clobber_bit_for_reg_name(ed.pin); - written = (bit != 0) && (exit_regs & bit) != 0; + bool written = false; + u16 bit = cfg->decl_pin_bit[i]; + if (bit != 0) { + written = (exit_regs & bit) != 0; } else { written = (exit_pm & bit_of(ed.entity)) != 0; } @@ -527,7 +568,7 @@ gb_internal void check_asm_cfg_analyse(AsmCtx *asm_ctx, CheckerContext *ctx, Ent if (diverging) { // No reachable path may return / fall off the end bool any_leak = false; for_array(bi, cfg->blocks) { - if (cfg->blocks[bi].reachable && check_asm_cfg_block_leaves(cfg, acc, cast(i32)bi)) { + if (cfg->blocks[bi].reachable && check_asm_cfg_block_leaves(cfg, cast(i32)bi)) { any_leak = true; break; }