Skip to content

ltm: emit_source_to_agg_link_scores re-parses and re-lowers an agg equation just to reach classify_reducer #983

Description

@bpowers

Problem

emit_source_to_agg_link_scores (src/simlin-engine/src/db/ltm/link_scores.rs, around line 2860) re-parses AND re-lowers an aggregate node's printed equation text through the entire front-end pipeline, purely to recover facts it already had.

The call site:

let agg_eqn = if agg.result_dims.is_empty() {
    LtmEquation::scalar(agg.equation_text.clone())
} else {
    LtmEquation::apply_to_all(agg.result_dims.clone(), agg.equation_text.clone())
};
let Some(agg_var) = reconstruct_ltm_var_lowered(db, &agg.name, &agg_eqn, model, project) else {
    return;
};
let Some(classified) = crate::ltm_augment::classify_reducer(&agg_var, from) else {
    return;
};

reconstruct_ltm_var_lowered (src/simlin-engine/src/db/ltm/parse.rs:126) runs the full boilerplate: parse_ltm_equation_for_model_with_ids (text -> Expr0, with module-ident and model-var-name scoping and implicit-module instantiation), then crate::model::lower_variable (Expr0 -> Expr2) against a freshly built ScopeStage0. All of that so classify_reducer (src/simlin-engine/src/ltm_augment.rs:4335) can walk the resulting Expr2 and hand back a ClassifiedReducer, of which the emitter consumes:

  • classified.kind (ReducerKind) -- checked against ReducerKind::Constant, and passed to the partial generators;
  • classified.name ("SUM" / "MEAN" / "STDDEV" / "RANK" / ...);
  • classified.body (the reducer's array-argument AST), fed into reducer_body_ctx_parts / ReducerBodyCtx.

All three are already known upstream:

  • ltm_agg::enumerate_agg_nodes (src/simlin-engine/src/ltm_agg.rs:593) is what decided this hoist in the first place, and it classified the reducer to do so.
  • ltm_agg::reducer_kind (src/simlin-engine/src/ltm_agg.rs:221) is the authoritative classifier; classify_reducer is documented as a thin reader over the same fact.
  • The reducer body is available already-lowered from the Expr2 the enumerator walked; AggNode::equation_text is itself a printed rendering (expr2_to_string) of that very subexpression.

So the pipeline is: Expr2 subexpression -> printed text (AggNode::equation_text) -> parse -> Expr0 -> lower -> Expr2 -> classify -> read back ReducerKind + name + body. A closed round trip through our own printer and parser to recover what the IR held before printing.

Why it matters

It is a deeper round trip than any of the ones Track A eliminated. The roundtrips-track-a2 branch (a stack on #980) removed eight print-and-reparse round trips in this subsystem; this one is untouched by that work and goes further than all of them -- it does not just re-parse, it re-lowers and re-derives a semantic classification.

It is on the LTM compile path, per agg node per source. For arrayed models with several aggregate nodes, each with multiple co-sources, this is a full front-end parse + lowering per (agg, source) pair. Related cost accounting lives in #655.

It re-derives from printed text information the IR already holds. That is exactly the drift/round-trip class the "one classifier family, never re-parse our own output" invariant on this subsystem exists to eliminate. Any parser/printer asymmetry, any change to expr2_to_string, or any divergence between ltm_agg::reducer_kind and classify_reducer's builtin set becomes a silent zeroed link score rather than a type error. #982 already tracks one live instance of exactly that divergence (two different reducer sets answer "is this reference inside a reducer?").

Failure mode is silence. Both fallible steps (reconstruct_ltm_var_lowered returning None on a parse/lower failure, and classify_reducer returning None) return early, emitting no source-to-agg link scores at all -- which silently zeroes the synthetic agg's loop score. The comment at the call site already documents one near-miss in this shape (an arrayed agg must be reconstructed as ApplyToAll over result_dims, because treating matrix[d1,*] as a scalar equation is a type error and the reconstruction would fail). That comment is evidence of the hazard: correctness now depends on the emitter rebuilding, by hand, the equation shape the enumerator already knew.

Components affected

  • src/simlin-engine/src/db/ltm/link_scores.rs -- emit_source_to_agg_link_scores (the call site)
  • src/simlin-engine/src/db/ltm/parse.rs -- reconstruct_ltm_var_lowered
  • src/simlin-engine/src/ltm_augment.rs -- classify_reducer / classify_reducer_in_expr
  • src/simlin-engine/src/ltm_agg.rs -- AggNode, enumerate_agg_nodes, reducer_kind

Suggested fix direction

Carry the classification forward instead of recovering it:

  1. Add the reducer kind + name (and the already-lowered reducer body / argument AST) to the AggNode that enumerate_agg_nodes produces. The enumerator has all three in hand at hoist time. AggNode already carries derived facts of this sort (result_dims, sources with per-source read_slice, array_valued_rank), so this fits the existing shape.
  2. Alternatively, thread a salsa-cached classification query through to the emitter, keyed on the agg node -- so the classification is computed once per agg rather than once per (agg, source).

Either way the acceptance property is: no emitter re-parses or re-lowers an equation to recover a fact the IR computed. Once AggNode carries the kind/name/body, reconstruct_ltm_var_lowered should have no remaining caller in link_scores.rs.

Note a second reconstruct_ltm_var_lowered caller at src/simlin-engine/src/db/ltm/loops.rs:1815 (agg-hop polarity recovery), whose comment says it "Mirrors emit_source_to_agg_link_scores' reconstruction". That one needs the whole lowered AST for source_to_agg_polarity, not just the reducer facts, so it is a somewhat different case -- but if AggNode grows a typed reducer representation, that site should be revisited in the same pass rather than left as the last text-round-trip holdout.

How this was discovered

Found during the Track A round-trip elimination work (branch roundtrips-track-a2, a stack on #980), while auditing remaining print-and-reparse sites in the LTM compile path. It is explicitly out of scope for that branch -- the fix touches AggNode's shape and the agg enumerator, not the equation-storage boundary Track A is changing, and it is deeper than the eight round trips that branch removes.

Relationship to existing work

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineIssues with the rust-based simulation engineltmLoops that Matter (LTM) analysis subsystemtech debt

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions