Conversation
…and bound-persistence charge
…cell registry, dry-run backend and diagnostics
…k and report, schedule and table gates
…unit cases
Three cases the batched DAG eval branch left failing are fixed.
1+2. The two [dryrun][peak] cases ("dryrun scratch-fold captures batched peak",
"dryrun peak is co-resident sum") measured a zero peak. test_eval_dryrun.cpp
opens with `#define SEQUANT_EVAL_TRACE 1` to flip Trace::Default to Trace::On
for its whole TU, which is what enables the compile-time-gated
note_working_set() calls inside the batched forest descent's nested re-entries
(the PeakSink folds those scratch high-water marks). Two things defeated it:
- Under the Unity build the file is concatenated AFTER siblings that already
pulled in eval.hpp, so the Trace enum was fixed at Off before the define was
seen. It now gets its own translation unit.
- make_batched_custom_evaluator returned a closure whose mangled name did not
encode the trace level, so the identically-typed instantiation compiled in
the (untraced) unity TU and the one compiled here emitted the same symbol
with different bodies and the linker kept one. The trace level is now an
explicit leading template parameter on make_batched_custom_evaluator and
make_evaluator (defaulted to Trace::Default, so every existing call site is
unchanged), forwarded to the nested evaluate_impl calls and to the
re-installed inner evaluators. Measured: the sink now reports 111.97 GB
against a 0.244 GB outer residency.
3. "home_scope is an identity accessor over sliced_modes" pinned the retired
meet-based definition. home_scope(n) is n->occurrence_home(), stamped per
occurrence by stamp_occurrence_homes, and is deliberately NOT the
stamp_lifetime_masks meet. The case is rewritten to pin the current contract on
two differently-sliced occurrences of one canonical node: home_scope equals
occurrence_home, keeps {i,j} and {i} respectively, while the meet folds both to
{i}; and stamp_lifetime_masks alone leaves home_scope empty.
The as-built design document is updated accordingly.
meter() already forces evaluate<Trace::On> so that note_working_set() observes
the PeakMonitor that MeterReport::peak_bytes is read from, but the batched
custom evaluator it installs on the forest_descent branch was left at
Trace::Default. Since the trace level now rides in that evaluator's type, that
had two consequences:
- In any build without SEQUANT_EVAL_TRACE -- mpqc's, i.e. the production
dry-run predictor -- the nested per-batch/per-member note_working_set()
calls were compiled out, so the predicted peak was blind to the
batched-inner scratch. On the C60 fixture that transient is 458x the outer
cross-batch residency, so the forest_descent prediction under-reported by
that much.
- meter() is inline and is instantiated from test_eval_dryrun.cpp (which
defines SEQUANT_EVAL_TRACE) as well as from the unity TU that holds
test_peak_profile.cpp and test_ordered_executor.cpp (which does not), so
the same mangled symbol carried two different bodies and the linker kept
one of them. test_peak_profile.cpp:494 asserts exact equality on a meter()
result, so the suite was link-order-dependent in principle.
Installing the evaluator at Trace::On explicitly fixes both; the ordered route
never installs this evaluator and is unaffected.
[dryrun-2iter-report] (water-20, aux, forest and ordered, cold and warm) is
byte-identical before and after -- builds, ops, exec, FLOPs and peak_bytes all
unchanged -- so the body the linker had been keeping was already the traced
one; the fix makes that deterministic rather than incidental. No test
expectation needed updating.
Also: reword the tests/unit/CMakeLists.txt note (the nested evaluate_impl()
re-entries are parameterized now) and correct the "~195x observed" figure in
test_eval_dryrun.cpp to the measured 458x.
All three pass locally only because the local Release build compiles
SEQUANT_ASSERT/TA_ASSERT out; CI configures SEQUANT_ASSERT_BEHAVIOR=THROW
for every build type and Debug additionally keeps TA_ASSERT.
- the two "mixed same-node open over an aux-contracted inner product"
fixtures put a_3 in the BRA of both h and t, which trips the tensor
network canonicalizer's strict bra<->ket policy. Relax the policy for
their scope, as the other multi-bra fixtures in this file already do;
the option only gates the assertion, so nothing else changes.
- shape_provider_general_product built its TA annotations by hand as
"i,j;a,b" and indexed the shape's tile range as {0,0}. The result of
I{a4<i2,i3>,a1<i1,i2>;i1,i2} * s{a2<i1,i2>;a4<i2,i3>} has THREE outer
modes -- i_1, i_2 and i_3, the pair-basis index of the contracted
composite, exactly as the case's own comment says -- so both were
rank-2 where the array is rank-3 and tripped TA_ASSERT in Debug. Use
the node's own annotation (already the evaluate() target) and a
three-index tile, and pin the outer rank so a layout change fails
loudly here instead of inside a TA tile lookup.
- eval_with_tiledarray's prod2 (a three-tensor chain) does not reproduce
the hand-written TA expression bit for bit: eval lays each intermediate
out in the node's canonical mode order while TA follows the written
annotations, so the two gemms accumulate in different orders. The
measured disagreement is 2.6e-16 RELATIVE, i.e. O(1e-14) absolute for
this fixture -- right at equal_tarrays' fixed 100*eps margin, which it
cleared only by luck (and does not on Linux/g++ + reference BLAS).
Compare relatively with four orders of headroom; a wrong contraction or
permutation shows up at O(0.1), not O(1e-13).
… jobs The CI sanitizer job ran 183 of its 264 ctest cases in 29.4 minutes and was killed at the 60-minute job limit; 18.5 of those minutes were spent in eleven of this branch's new cases, each of which builds a schedule out of tests/unit/data/csv_ccsd_doubles_residual_df.txt (or the C60 giant term). They cost 2-4 seconds natively and 50-145 seconds EACH under ASan/UBSan; under valgrind the whole binary stopped producing output after four minutes and was still running 49 minutes later. Master's sanitizer job took 45 minutes and its valgrind job 15. Gate those thirteen cases on SEQUANT_SKIP_LONG_TESTS, which the workflow already sets for exactly the valgrind and sanitizer configurations. Deliberately NOT on SEQUANT_INTERNAL_SKIP_LONG_TESTS, which also fires for every Debug build: these cases are the ones that exercise the assertion-heavy Debug paths, so a plain Debug build must keep running them. Also give test_ordered_executor.cpp (5.4 kLOC) and test_ordered_schedule.cpp (3.2 kLOC) their own translation units. Concatenated with their seven siblings into one Unity TU they make a single cc1plus invocation whose peak memory a 16 GB 2-core runner cannot afford next to a second parallel compile: both CI runs of this branch died mid-compile of that target's unity_0 with the hosted runner's out-of-memory signature (SIGTERM plus "the runner has received a shutdown signal"), 14 minutes into the Debug + CMAKE_UNITY_BUILD job.
The sanitizer configuration finished its last green run at 59.9 minutes against the 60-minute cap: 28.2 min building and 28.9 min in ctest. Of those 28.9 test minutes, 25 are spent in eight long-standing cases -- sequant/unit/mbpt_cc alone is 10 minutes under ASan, and the srcc/ucc/stcc integration tests another 10 -- so the pressure is not from any single change and gating more cases buys about two minutes. Give the matrix headroom rather than lose runs to a six-second margin.
Copying a FullBinaryNode deep-copies its whole subtree, so every place that kept a node BY VALUE cost O(subtree) allocations per node visited. That was tolerable while binarize produced one small tree per summand; it is quadratic now that an equation is one left-leaning Sum-tree whose left spine carries one node per summand (thousands for a UCC BCH energy expansion). Three such places: - cache_manager()'s volatile/persistence overload keyed its DAG-walk maps (counts, volatile_of, persistent) on nodes by value. They now key on non-owning node pointers -- the hasher and comparator already hash and compare by node CONTENT through a pointer, so dedup is unchanged -- and only the nodes actually selected for caching are copied, at the end. A static_assert pins the (pre-existing, and already relied on by the min_repeats-only overload) requirement that the node range yield references to nodes outliving the call. - evaluate_impl()'s explicit evaluation stack stored a Node in every Frame, so a walk down the spine held one deep copy per frame. Frames now hold a pointer into the tree being evaluated. - CacheManager's lookups (access_at/access/exists/store_and_access, the life / alive / persistent / resident_in_chain / entry_size_in_bytes probes, and tally_build) took a CachedValue, which is implicitly constructed from a node BY COPY -- a deep copy per probe, including on the wet path where tally_build's tally is disabled. They now take the node and probe the map heterogeneously; CachedValueHasher / CachedValueEqual gain the bare-node overloads that makes that a no-copy lookup. Behavior is unchanged: same entries cached, same persistence classification, same results. MPQC's spin-free energy_comm_rank=4 UCC validation cases (h2o-ucck-2-bch2e4-*), which exhausted a 64 GB ASan allocator inside FullBinaryNode::deep_copy, now run to completion.
AGENTS.md (and PR #612, which converted the tree) requires every throw in this repository to be sequant::Exception or a class derived from it. The batched DAG eval work predates that rule and added 23 throws of std::runtime_error / std::logic_error / std::invalid_argument / std::out_of_range: in optimize/single_term_detail.hpp, eval/cache_manager.hpp, eval/cell_table.hpp, eval/cell_table_builder.hpp, eval/cell_registry.hpp, eval/eval.hpp, eval/ordered_executor.hpp and eval/backends/dryrun/result.hpp. They become throw Exception(...), and each file that names Exception now includes utility/exception.hpp directly. The unit tests that assert on those sites follow: the CHECK_THROWS_AS(..., std::invalid_argument) in test_optimize.cpp and test_eval_dryrun.cpp and the two std::logic_error ones in test_eval_ordered.cpp now expect sequant::Exception. Left alone: the std::out_of_range check in test_eval_dryrun.cpp and the catch around SizeRegime::extent in dryrun/result.hpp. That exception comes out of std::map::at, not a throw statement, so the rule does not reach it.
ScopeBlock holds container::vector<Step> (a std::vector) while Step -- which
wraps std::variant<BuildStep, ScopeBlock> -- is only forward-declared; Step's
definition has to follow ScopeBlock's because the variant needs ScopeBlock
complete. std::vector tolerates an incomplete element type only up to the
first use of one of its members.
Defining Step instantiates std::variant<BuildStep, ScopeBlock>, which asks
whether ScopeBlock is trivially copyable/destructible. With ScopeBlock's
special members implicitly declared that question defines them, which in turn
instantiates std::vector<Step>'s -- and Step is still incomplete right there,
inside its own definition. libstdc++ does pointer arithmetic on Step* in
those members, so clang reports
stl_vector.h:369:35: error: arithmetic on a pointer to an incomplete type
'sequant::eval::Step'
ordered_schedule.hpp:97:32: note: in defaulted default constructor for
'std::vector<sequant::eval::Step>' first required here
on mpqc4's GitLab CI (clang++-19, unity build). libc++ has no such
arithmetic, and GCC's instantiation timing differs, so neither shows it.
User-declaring the six special members and defining them "= default" below
Step is the standard fix: the variant now answers the triviality question
from the declarations alone, and the definitions land where Step is complete.
ScopeBlock stops being an aggregate, which is fine -- every construction in
the tree is ScopeBlock{} or member assignment, never aggregate or designated
initialization.
evaleev
force-pushed
the
feature/batched-dag-eval
branch
from
September 13, 2026 19:07
a11cafa to
3681645
Compare
Member
Author
|
@Krzmbrzl would you please numerically verify if regenerated nevpt2 exports are correct? These are the only failing cases at the moment: https://github.com/ValeevGroup/SeQuant/actions/runs/34776721253/job/103776060655?pr=613#step:14:741 |
…<Step> must not be instantiated before Step is complete; clang with libstdc++)
Collaborator
|
Why have they changed again? I thought we sorted the symmetry thing out 👀 But yeah, will do |
Member
Author
|
For the nevpt2 verification: this is not the symmetry change again. The only mechanism behind the fixture diff is the CSE comparator: Net effect on the emitted ITF: one fewer alloc, contraction and load, two fewer drops, same 65 stores and 49 CSE statements, no term added or removed, no coefficient changed; 22 of the 24 diff hunks are pure renumbering. Disabling only that commutative branch reproduces the old output byte for byte. The diff hunks with the mapping are in the PR branch's ctest output ( |
The code has been numerically verified to still yield the correct result.
Krzmbrzl
requested changes
Sep 14, 2026
Comment on lines
136
to
158
| // A Product (contraction) is commutative: the binarizer may emit the same | ||
| // contraction as (X,Y) in one term and (Y,X) in another, and the hash and | ||
| // canonical connectivity graph both fold that operand order -- so the | ||
| // child comparison must fold it too, else two swapped occurrences of the | ||
| // same value are wrongly split into two cache entries (built twice). | ||
| // Match the children as an UNORDERED pair. The recursive child comparison | ||
| // is still REQUIRED and is NOT redundant with the graph check above: the | ||
| // connectivity graph encodes only the immediate two factors' | ||
| // connectivity, not each factor's recursive build, so two products with | ||
| // the same immediate graph but different sub-values must still be told | ||
| // apart -- and are, because the unordered match fails when no child | ||
| // pairing is equal. Product operand subtrees are bounded in depth (a | ||
| // contraction of a fixed set of factors), so they stay recursive. | ||
| if (lhs->op_type() && *lhs->op_type() == EvalOp::Product) { | ||
| bool const in_order = (*this)(lhs.left(), rhs.left()) && | ||
| (*this)(lhs.right(), rhs.right()); | ||
| bool const swapped = (*this)(lhs.left(), rhs.right()) && | ||
| (*this)(lhs.right(), rhs.left()); | ||
| if (!in_order && !swapped) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
Collaborator
There was a problem hiding this comment.
To me it seems that doing things this way make the calls really expensive. This now scales exponentially for every factor in a product.
The better fix would be to ensure that the assumption of the previously existing implementation actually holds. That is, equations must not be factored as AB in one place and BA in another.
Presumably, this requires tweaking with how we do comparisons. Using hashes makes for a nifty optimization but also makes it hard to impossible to get any specific order. A change in the comparison philosophy could also address #614. My proposal would be to do deep comparison when doing operator<=> and only use the hash-based shortcut for equality comparison. Given that the latter is the most frequent anyway, this should retain most of the performance boost whilst giving us a predictable and controllable sort order.
Member
Author
There was a problem hiding this comment.
You're right, and the first version of the fix was still wrong on exactly the point you raised, so thank you for pushing on it. Two commits, f1b83fb and 4b96590.
Evaluation order stays exactly as the DP/binarizer emits it (cost, and peak in particular, is order-dependent, so the schedule has to be); what was wrong is that node identity was reading that order. A Product node's children are now compared through a canonical view (canonical_children, ordered by an O(1) key: scalar last, then ascending node id, ties keep emitted order), and the comparator compares the two views pairwise, one recursion per child, so it is linear unconditionally; nothing is reordered or cached on the node. The value key the DAG scheduler derives gets the same treatment. Details, the O(n^2) trap the first version had, and the evidence for the extra folds are in the PR description under "Node identity vs evaluation order".
Could you take another look?
…esult, as the DP does DryRunOps::prod priced each product op as exec_cost(flops, memsize(left), 4096) -- the LEFT operand's realized footprint plus a 4096-byte placeholder for the right operand, and nothing for the result. roofline_op_cost's `traffic` is the COMPULSORY single-pass data movement of one contraction: read both operands, write the result. That is what its own doc says, what the design of record says (doc/dev/specs/2026-06-23-roofline-tiebreak-cost.md section 2.1: traffic = S[lp] + S[rp] + S[n]), and what the optimizer's DP actually passes (PeakModel::relax and BatchedPeakModel::relax). The finite-cache re-read effect is the separate Hong-Kung branch inside the same max, not this term. So the replay both under-counted the traffic and made `exec` depend on which operand happened to land on the left -- a swap moved it by the whole ratio of the two operand footprints, at identical flops. CostModel::exec_cost now takes the three footprints explicitly (left_bytes, right_bytes, result_bytes) and sums them; DryRunOps::prod passes each at its REALIZED (sliced) extent, sized against its own index list with its own positional overrides -- the result footprint being exactly the one make_dryrun_result hands the result token. The scalar-operand early return is unchanged. Doxygen rewritten accordingly. New test (tag [dryrun-costmodel][roofline]) takes a 4000-element x 100-element product in BOTH operand orders and asserts the per-op exec is identical and equals roofline_op_cost at |L| + |R| + |out| elements, with a non-vacuity guard that the case really is bandwidth-bound. [dryrun-2iter-report] water20/aux_occ: builds, ops, FLOPs and peak_bytes are unchanged; exec rises 22-26% on all four rows (forest cold 8.504e14 -> 1.0389e15, forest warm 8.085e14 -> 9.969e14, ordered cold 7.254e14 -> 8.934e14, ordered warm 4.874e14 -> 6.121e14). No *.expected fixture changes; all sequant/cost_analysis ctest entries pass.
A contraction is commutative, so the single-term DP is free to hand binarize its two operands in either order -- and does: which of a pair lands on its stack first decides the eval tree's left/right. The node id (hash_value) and the canonical connectivity graph both fold that order, so the two spellings ARE one value, but an ORDERED child comparison split them into two cache entries. 6ecb6d5 made TreeNodeEqualityComparator fold them by matching the two children as an UNORDERED pair, which recurses twice per child and is therefore exponential in product depth (PR #613 review). Evaluation ORDER stays exactly as the DP/binarizer emits it -- cost, and peak in particular, is order-dependent, so the schedule must be order-dependent and the tree must not be permuted. What was wrong is that node IDENTITY was reading that order. So identity now reads the children through a canonical VIEW instead: canonical_operand_cmp(a, b) orders two operands by a key that is a function of their VALUES only: a scalar operand sorts last (matching how binarize already builds a `scalar * tensor` node), then ascending node id, then -- on a node-id tie -- the equality comparator itself, so the order can never separate two operands the comparator would fold; only a genuine 64-bit collision between distinct values reaches a deterministic deep tie-break (graph cmp, canonical indices, phase, label, then the children, itself read through the canonical view). canonical_children(n) returns n's two children through that order. The node is NOT modified and nothing is cached on it: in every case that occurs in practice the view costs one comparison of two already-computed hashes. TreeNodeEqualityComparator then compares canonical_children(lhs) against canonical_children(rhs) pairwise -- ONE recursion per child, linear -- and the either-pairing match is gone. The iterative left-spine loop for deep Sum trees is unchanged, as is the ordered comparison of every non-Product node. The same reasoning applies to the VALUE key the batched DAG scheduler derives from a node and its operands' keys (peak_profile's compute_dag_boulevard, and lifetime_mask's value_key_impl fallback): it combined the two operand keys in emitted order and so did NOT fold the swapped spellings even where the node cache did. For a Product node the two operand keys are now combined in ascending order. On the water-20 aux+occ dry-run report the ordered executor does 10 FEWER builds in both the cold (1542 -> 1532) and the warm (1286 -> 1276) iteration, with lower FLOPs (6.938e14 -> 6.843e14 cold) and lower peak (214.65 -> 214.30 GB cold); the forest rows are unchanged in builds, ops and peak. The generated NEVPT2 ITF is byte-identical, as it must be since nothing is reordered.
Review follow-up to f1b83fb. canonical_operand_cmp resolved a node-id tie by running a FULL equality comparison of the two operand subtrees, and only then a deep tie-break. That runs at every comparator visit and at every canonical_children call, so with hash-equal operands nesting the comparator is T(n) = 4 T(n/2) = O(n^2) -- a `t2 * t2` self-contraction is enough to reach it, which makes the header's "one comparison of two hashes in every practical case" wrong. A node-id tie now resolves to 0, i.e. to the EMITTED order, and detail::deep_tie_break / detail::cmp_canon_indices are deleted with it. That is exactly right for the case that occurs: two operands that are ONE value have one node id, either order is canonical for them, and an ordered comparison of two swapped spellings succeeds whichever way each is read. The only other way to reach the tie is a genuine 64-bit collision between DISTINCT values, and there the cost is a MISSED fold (the two spellings stay two cache entries, as they were before any folding existed), never a wrong one -- TreeNodeEqualityComparator still compares both subtrees in full and rejects a mismatch. canonical_operand_cmp is now O(1), reading nothing but the two nodes' own already-computed data, and the comparator is provably linear. With steps 3-4 gone, canonical_operand_cmp no longer instantiates a default-constructed TreeNodeEqualityComparator, which had discarded the index list SubexpressionIdentifier configures it with. Also: canonical_children returns references into its argument, so a deleted `TreeNode const&&` overload now rejects a temporary; and three comments are added where the reasoning is easy to undo -- - peak_profile.hpp and lifetime_mask.hpp, at the two value-key sites: they order the two operand KEYS, NOT via canonical_children, because a node id ties whenever the two operands are one value while their keys can still differ there (one value home-sliced two ways is two keys). Both rules are value-determined and do not need to agree; unifying them would reintroduce an emitted-order dependence. - ordered_schedule.hpp, at the production-edge guard: it is what keeps the order-independent Product identity safe. A consumer cell can now own occurrences whose trees carry the two operands on opposite legs; facts are recorded only from the canonical (front) production occurrence and cell_table_builder derives `my_leg` from those (operand, consumer) facts, so every leg comes from one tree. Relaxing the guard would cross them. Gates unchanged: the water-20 aux+occ dry-run report still reads 1532 (cold) / 1276 (warm) ordered builds, `ctest -R 'nevpt2|cost_analysis'` is green with a byte-identical generated ITF, and the full non-hidden unit suite passes.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds phase-one batched DAG evaluation with ordered scheduling, explicit cell lifetimes, dry-run support, and backend integration.
Changes:
- Adds cost-model-driven batched scheduling and legality/lifetime analysis.
- Adds table-driven execution, assembly, persistence, and dry-run diagnostics.
- Updates spin APIs and expands unit coverage.
File summaries
| File | Description |
|---|---|
| tests/unit/test_tensor_network.cpp | Updated as part of this pull request. |
| tests/unit/test_spin.cpp | Updated as part of this pull request. |
| tests/unit/test_space.cpp | Updated as part of this pull request. |
| tests/unit/test_slicing_signature.cpp | Updated as part of this pull request. |
| tests/unit/test_eval_expr.cpp | Updated as part of this pull request. |
| tests/unit/test_eval_btas.cpp | Updated as part of this pull request. |
| tests/unit/test_dag_scope.cpp | Updated as part of this pull request. |
| tests/unit/test_canonicalize.cpp | Updated as part of this pull request. |
| tests/unit/data/legality_cross_iteration.txt | Updated as part of this pull request. |
| tests/unit/data/csv_ccsd_singles_residual_df.txt | Updated as part of this pull request. |
| tests/unit/data/csv_ccsd_energy_df.txt | Updated as part of this pull request. |
| tests/unit/CMakeLists.txt | Updated as part of this pull request. |
| SeQuant/domain/mbpt/convention.hpp | Updated as part of this pull request. |
| SeQuant/domain/mbpt/convention.cpp | Updated as part of this pull request. |
| SeQuant/core/utility/expr.cpp | Updated as part of this pull request. |
| SeQuant/core/tensor_network/vertex_painter.hpp | Updated as part of this pull request. |
| SeQuant/core/tensor_network/vertex_painter.cpp | Updated as part of this pull request. |
| SeQuant/core/tensor_network/v3.hpp | Updated as part of this pull request. |
| SeQuant/core/tensor_network/v3.cpp | Updated as part of this pull request. |
| SeQuant/core/tensor_network/typedefs.hpp | Updated as part of this pull request. |
| SeQuant/core/optimize/single_term.hpp | Updated as part of this pull request. |
| SeQuant/core/optimize/single_term_detail.hpp | Updated as part of this pull request. |
| SeQuant/core/optimize/options.hpp | Updated as part of this pull request. |
| SeQuant/core/optimize/optimize.hpp | Updated as part of this pull request. |
| SeQuant/core/optimize/optimize.cpp | Updated as part of this pull request. |
| SeQuant/core/logger.hpp | Updated as part of this pull request. |
| SeQuant/core/index_space_registry.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/value_node_map.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/value_id.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/slicing_signature.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/schedule_dump.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/result.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/peak_monitor.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/node_batch_annotation.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/lifetime_mask.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/fwd.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/forest_combine.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/eval_node_compare.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/eval_expr.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/eval_expr.cpp | Updated as part of this pull request. |
| SeQuant/core/eval/dag_scope.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/cache_manager.cpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/tiledarray/eval_context.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/tiledarray/array_ops.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/tapp/result.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/dryrun/size_regime.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/dryrun/eval_expr.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/dryrun/cost_model_object.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backends/btas/result.hpp | Updated as part of this pull request. |
| SeQuant/core/eval/backend_array_ops.hpp | Updated as part of this pull request. |
| SeQuant/core/binary_node.hpp | Updated as part of this pull request. |
| SeQuant/core/batch_policy.hpp | Updated as part of this pull request. |
| external/versions.cmake | Updated as part of this pull request. |
| doc/dev/batching-mode-terminology.md | Updated as part of this pull request. |
| CMakeLists.txt | Updated as part of this pull request. |
| .gitignore | Updated as part of this pull request. |
| .github/workflows/cmake.yml | Updated as part of this pull request. |
Review details
Suppressed comments (4)
SeQuant/core/eval/eval_node_compare.hpp:165
- Although the Sum spine is unwound iteratively,
FullBinaryNode::size()traverses the entire subtree on every iteration. For equal, hash-equal left-folded Sums this repeats sizes N + (N-1) + ... and turns the intended linear comparator into O(N²), which will dominate CSE on the large equations this change targets. Avoid recomputing full subtree sizes at each spine node, for example by comparing structure in one pass or carrying/caching the needed sizes.
SeQuant/core/optimize/optimize.cpp:306 ex<Sum>(std::move(new_sum))moves the summand storage beforerekey_ontoruns on the next line. In the no-reorder path,rekey_ontothen indexesnew_sum.summand(p), so the batched annotation map can be lost or throw; rekey from a still-live final/sourceSum(or otherwise preserve the source before moving it).
SeQuant/core/optimize/single_term_detail.hpp:108- This branch now throws whenever composite indices are passed without
inner_pow, but the public documentation above (theinner_aware_volumenote and theflops_counter/memsize_counter/footprint_counterparameter docs) still promises fallback sizing viaixex. Callers following the documented default behavior now receive an exception; update those contracts to describe the requiredinner_powand failure mode.
SeQuant/core/utility/expr.cpp:407 - The proto-aware validation mode is selected only from
sum.summand(0). A valid CSV sum can have a later summand containing a composite/proto index while the reference summand uses the corresponding bare indices; in that case this stays on the slot-onlyget_unique_indicespath and can reject the sum as having inconsistent external indices. Detect proto indexing across all summands before choosing the validation branch.
- Files reviewed: 72/87 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+460
to
+465
| SEQUANT_ASSERT(append || prepend); | ||
| if (append) | ||
| lohi.second = block_hi; | ||
| else | ||
| lohi.first = block_lo; | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed in c75136d: a scatter block that neither appends nor prepends to the current coverage (gap or overlap) now throws sequant::Exception, independent of the assertion policy.
Comment on lines
+46
to
+47
| [[nodiscard]] std::size_t color() const { | ||
| return (depth << 12) | static_cast<std::size_t>(loop_slot); |
Member
Author
There was a problem hiding this comment.
Fixed in c75136d: color() is gone; ordered_n_batches_by_loop() and the other loop-keyed tables key on LoopKey itself through std::hash (full-width fields, no packing), so slot numbering is unbounded without collisions.
Comment on lines
+518
to
+519
| child_recs.push_back(self(self, n.left(), child_ectx, child_opener)); | ||
| child_recs.push_back(self(self, n.right(), child_ectx, child_opener)); |
Member
Author
There was a problem hiding this comment.
Fixed across c75136d (this prepass plus the analogous walks in lifetime_mask.hpp, legality.hpp, value_node_map.hpp), f07a116 (meter.hpp, cache_manager.hpp, the volatility walks; FullBinaryNode::size() now cached instead of recursive) and f0b64c9 (the four remaining recursive walks in eval.hpp, including subtree_any on the ordered path). All are explicit-stack traversals now; a hidden N=20000 spine test in test_peak_profile.cpp exercises the ordered prepasses, and the value->node bridges hold forest nodes by pointer (f0b64c9), which took the same spine from 13 GB RSS to 0.4 GB.
| o.reduced_slot = r.reduced_slot; | ||
| return o; | ||
| }; | ||
| auto const it = hash_to_cell.find(r.key); |
Member
Author
There was a problem hiding this comment.
Fixed in c75136d (propagated through the rest of the ordered path in f07a116): value grouping now buckets by hash and confirms membership structurally (same_value), so two distinct values with colliding hashes get distinct cells; the cells carry schedule-unique salted keys that are re-stamped onto the nodes, and b910a0c adds a test-side check that every non-leaf cell has exactly the production sites the schedule's own well-formedness rules require.
…confirm value cells structurally Four Copilot review findings on PR #613, all with a unit test. 1. dryrun/result.hpp write_into_slice: a block that neither appends after nor prepends before the assembled coverage was caught only by a SEQUANT_ASSERT. Asserts are compiled out in non-Debug builds (CI builds with SEQUANT_ASSERT_BEHAVIOR=THROW rather than relying on them), so a gapped or overlapping scatter updated the coverage from stale data and the dry run accepted -- then mis-sized -- an incorrect scatter. Now a sequant::Exception naming the mode, the offending block range and the coverage it fails to abut. The assert is dropped: the throw subsumes it. 2. dag_scope.hpp LoopKey: the packed color() (depth << 12 | loop_slot, with only a comment asserting loop_slot < 4096, while peak_profile.hpp hands out slots from an unbounded per-space counter) is REMOVED rather than bounds-checked. Its single user, ordered_n_batches_by_loop, now keys its map on the LoopKey itself through a std::hash specialization plus the existing operator==; a hash/equality pair has no bit budget to overflow, so there is nothing left to guard. 3. Forest-walking prepasses on the ordered path recursed to the depth of the residual's in-place Sum tree (one node per summand, thousands of terms), overflowing the call stack before the stack-safe executor is reached. Converted: - compute_dag_boulevard's post-order linearizing walk -> explicit frame stack (pre ids, points, rec order and consumer_point stamps are a faithful transcription of the recursion); - stamp_residency_impl and stamp_occurrence_homes -> explicit stacks (right child pushed before left, preserving pre-order); - value_key_impl -> left spine unwound, right children still recursive (the TreeNodeEqualityComparator pattern); - analyze_legality's node_of walk and both build_*_node_map walks -> iterative pre-order. cell_table_builder's emit_cells recurses over the ScopeBlock tree only (depth = loop nesting, bounded) and is left alone. 4. peak_profile.hpp value-cell grouping keyed on the 64-bit value key alone, so a collision between distinct values merged them into one cell and the schedule would build one and read it as the other. The map now holds a bucket of cell ids per key and confirms a hit structurally: node identity via TreeNodeEqualityComparator (canonical child view), the per-position loop slots and batch-reduced slot arity the key folds in, and the operands by CELL -- inductive, since recs are in post-order. Gates: full non-hidden unit suite green under Debug+ASan (14166 assertions, 294 cases); [dryrun-2iter-report] builds/ops/FLOPs/peak/exec byte-identical to HEAD for water20 aux (ordered 233/183) and aux_occ (ordered 1532/1276); ctest -R 'nevpt2|cost_analysis' 15/15.
…volatility walks Fix round 1 on the four Copilot review findings (c75136d). 1. The value-cell split did not propagate. ValueCell::key was still r.key, so the two cells a confirmed structural mismatch opens shared one value id, and every downstream resolution -- ordered_schedule's value_id_of / hash_to_rich, legality's CellLegality::hash, the executor's key -> node maps -- is a first-wins emplace on value_key_of(ValueCell), collapsing both back onto the first cell's value_id. Each cell now takes a value id that is unique across the schedule (salted with hash::combine until free, 0 reserved), and every occurrence's EvalExpr::value_key is re-stamped with it so value_key_of(node) and value_key_of(cell) -- which those maps join on -- keep agreeing. Nothing is salted when keys do not collide, so the ordinary case is bit-for-bit as before (the dry-run report confirms). 2. Cost of the structural confirmation. FullBinaryNode::size() walked the subtree (and heap-allocated) on every call, and TreeNodeEqualityComparator calls it per node before the cached node id. size() is now maintained in O(1) at construction / assignment (assignment refreshes ancestors too, since left()/right() permit replacing a child in place), and the comparator tests the cached node id first. Two more quadratics surfaced while measuring and are fixed with it: - stamp_residency_impl's pass 2 re-found every occurrence in the structurally-keyed meet, and a hit there walks the whole subtree; pass 1 now records each occurrence's meet entry, so pass 2 does no lookup; - analyze_legality's node_of held forest nodes BY VALUE, and Node's copy ctor deep-copies the subtree -- O(nodes x subtree) memory; it holds pointers now (the forest is the caller's and outlives the call). Together: the deep-spine test with compute_dag_boulevard at N=20000 went from OOM-killed (>150 s, 15+ GB) to 0.90 s / 4.0 GB, and now also covers analyze_legality. 3. The ordered dry-run REPORT path still recursed to spine depth: dryrun::compute_volatility (assemble_report's classifier, used for the ordered arm) and cache_manager's NV/V frontier walk. Both are explicit frame stacks now -- faithful transcriptions, so volatile_of, counts and persistent come out identical. Minor, same round: ordered_schedule.hpp no longer cites the deleted LoopKey::color; same_value documents that it compares against a cell's REPRESENTATIVE only (non-transitive on purpose -- the worst case is a missed fold, never a wrong merge, and all-pairs would be quadratic for nothing); the collision test gained a case whose two products have IDENTICAL operands, so the child-cell and slot comparisons all tie and only TreeNodeEqualityComparator can separate them; both collision tests now run ordered_schedule_dep_graph, analyze_legality and build_ordered_schedule and assert two value ids survive to each. The one-line reformat of FullBinaryNode::tikz is the pre-commit clang-format hook bringing an untouched declaration into compliance, not an intentional edit. Known follow-up (measured, not fixed here): build_value_node_map / build_value_key_node_map hold nodes by value too, so each costs O(nodes x subtree) memory -- 1.8 GB at spine depth 500, 15.7 GB at 2000, OOM past that -- and evaluate_ordered_schedule builds one per run. Fixing it means changing their return type to hold pointers, ~30 call sites, so it is left for its own change; the deep-spine test documents the numbers and excludes them. Gates (cmake-build-relwithdebinfo): full non-hidden suite All tests passed (318024 assertions in 295 test cases, 58 s); every requested tag green; [dryrun-2iter-report] builds/ops/FLOPs/peak/exec byte-identical to c75136d for water20 aux (ordered 233/183) and aux_occ (ordered 1532/1276); ctest -R 'nevpt2|cost_analysis' 15/15.
… the last spine recursions build_value_node_map / build_value_key_node_map held each forest node BY VALUE, and FullBinaryNode's copy constructor deep-copies the whole subtree, so one entry per node cost O(nodes x subtree) memory -- 1.8 GB for a 500-summand Sum spine, 6.9 GB at 1000, 15.7 GB at 2000, OOM past that -- for data the forest already holds, and evaluate_ordered_schedule builds one per run. Both now return ValueNodeMap<Node> = unordered_map<size_t, Node const*>. Every builder's forest outlives its map: it is a parameter of the enclosing call (run_ordered_schedule_pre_results, which also passes the map by const reference into the block walk) or a local of the test case declared before the map. Two test-side structures that copied a map value into a longer-lived holder (persistent_b1, dead_transient in test_ordered_executor.cpp) hold Node const* now, same argument. The maps and analyze_legality's node_of gain the range-yields-references static_assert cache_manager() already carries, since the entries are addresses. Also, from the same review round: - FullBinaryNode's move assignment refreshed the cached size_ chain of the TARGET only. Moving from a node that is itself someone's child (std::move(n.parent().right()), export.hpp's prune_scalar_node) left the source's ancestors over-counting the subtree they no longer hold. The source's chain is refreshed too, and right_ is parked in a temporary alongside left_ so the source stays alive through the function even when it was owned by one of the target's former children. - subtree_any, make_batched_scratch's signature walk, place_at_this_level's collect and the replay group's `contains` are explicit stacks now. All four can be rooted at the node `evaluate` was called on -- a forest root, i.e. the residual's single in-place Sum tree -- and subtree_any is on the ordered path as well (volatile_of resolves a value id to its node). Each pushes the right child before the left, so the visit order is the recursion's pre-order and the recorded signatures, counts and target order are unchanged. - read_operand documents that its descent is bounded by cell boundaries, not tree depth. Tests: the deep-spine test drives both maps and checks an entry is the forest node's own address; the value-cell split check replaces the tautological num_values comparison with the schedule's actual productions (one BuildStep per non-leaf cell); test_binary_node asserts node counts after in-place child replacement and after a child steal. water20 dry-run report numbers (both batch modes) byte-identical, ordered builds still 1532/1276 for aux_occ.
…t-root copies Follow-ups from the re-review of the value->node pointer conversion. The per-cell assertion in check_split_propagates encoded an invariant build_ordered_schedule does not hold: a value with any Reduction or LoopCarried per_axis role has NO BuildStep anywhere -- it is produced as an AccumulateSum / AccumulateScatter output of each escaped block (ordered_schedule.hpp's own contract), as the non-hidden per-nest-split tests show. It passed only because both collision fixtures have no batchable index, and would have gone red on any fixture that does, reading like a value-cell collapse that is not one. It now compares detail::collect_production_ids -- BuildSteps plus block outputs, the notion well_formed uses -- against the non-leaf cell ids, as SETS, because a multi-level escape chain legitimately lists one value at several depths and a built value may also escape through its own chain. The hand-rolled recursive collector is gone with it. FullBinaryNode's move assignment returns early on self-move. Parking right_ in a temporary alongside left_ (needed so the source survives the source-side size refresh) made self-assignment lossier than before: both children went into the temporaries while node.left_/right_ -- the same members -- read null, leaving a childless leaf and destroying both subtrees at scope exit. The early return also retires the &node != this test at the refresh and the self-move of T. The comments cited export.hpp's prune_scalar_node, which does not exist; the function is prune_scalar_factor (export.hpp:345, the move-from-a-child at :393-396). Fixed in both source references. Also: - The three range-yields-references static_asserts now check range_reference_t<R const>, spelled identically in all three places: the walks iterate a range bound as R const&, so a range that yields references only when mutable must not pass. - subtree_any's stack is a container::svector<Node const*, 32>: the hot callers are per-node (place_at_this_level's collect) and per-value (the ordered path's volatile_of), and the recursion this replaced allocated nothing. - The pointer identity the conversion is about -- an entry IS the forest node, not a copy of its subtree -- is pinned by a new NON-hidden test; only the deep-spine case that carries the memory numbers stays hidden. - read_operand's comment says its cell-boundary bound is a schedule property (a fetch miss falls through to the transient arm, which descends the whole subtree; the topological order is what makes the miss impossible), not a structural one. - Four remaining forest deep-copies are pointers now: the three container::svector<node_t> root lists on the ordered path (both run_ordered_schedule_pre_results and evaluate_ordered_schedule cloned every root's whole subtree once per call, evaluate_ordered_multiroot twice), for which combine_forest_roots takes svector<node_t const*>, plus the twin find_parent holders and one walk stack in the tests -- the twin searches are iterative now too. - Two dead build_value_node_map locals in test_eval_ta.cpp are deleted. water20 dry-run report numbers (both batch modes) still byte-identical, ordered builds 1532/1276 for aux_occ.
The iterative find_parent replacement in test_legality.cpp and test_ordered_schedule.cpp pushed every forest tree and then popped LIFO, so the LAST tree was searched first -- the recursion it replaced went forward, and the comment claimed a plain pre-order. The trees are pushed in REVERSE now, so tree 0 pops first; each node's right child was already pushed before its left, which is the correct order within a tree. The comment states both halves. test_legality.cpp's <functional> include went unused when the std::function search became a stack walk; dropped.
Conflicts: tests/unit/test_cache_manager.cpp (master's (void) on the discarded store() result meets this branch's store_and_access() rename), tests/unit/test_optimize.cpp (master's capture-less batch_fn meets this branch's PeakBatchedModel field list, which has no is_batchable). Master (#595) compiles the unit tests with -Werror -Wall -Wextra, so the merged tree also needed: a default member initializer on PeakModel/PeakBatchedModel::inner_pow (aggregate initializations in the optimizer tests omit it; an empty inner_pow still throws on a composite index, as documented), and three test-only cleanups (unused batch_fn and nblocks, (void) on a [[nodiscard]] store_and_access()).
Member
Author
|
Merged master (4112eb6) into this branch as e13146f rather than rebasing, so the commit shas cited in the review threads stay valid. Two test-file conflicts resolved; master's -Werror on the unit tests (#595) needed a default member initializer on PeakModel/PeakBatchedModel::inner_pow plus three test-only cleanups (details in the merge commit message). RelWithDebInfo build clean, unit suite green (318143 assertions / 296 cases). |
GCC's -Wmissing-field-initializers (an error on CI since #595) fires on the designated initializers in the optimizer tests that omit inner_pow; clang does not warn on designated init, which is why the merge gate was clean locally. An empty inner_pow was already the effective default (it throws on a composite index, as documented), so this only makes the default explicit, as PeakModel/PeakBatchedModel already do.
… -Werror) log::EvalStat::mem_left/mem_right and eval::Read::invariant_on were the last fields omitted by aggregate/designated initializers that GCC's -Wmissing-field-initializers reports (clang stays silent); found by a g++-15 -fsyntax-only sweep of all 116 SeQuant and unit-test TUs with the recorded flags, which is now clean. No behavior change: the defaults are the values those initializers already produced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of batched array DAG evaluation for factorized CC equations.
What this adds (see
doc/dev/specs/2026-09-12-batched-array-dag-eval-as-built.mdfor the as-built design):optimize/cost_model.hpp: batched DP over ordered cells with per-node external loop opens and bound-persistence charge; perf-first(peak, flops, nsl)frontier.eval/peak_profile.hpp,lifetime_mask.hpp,legality.hpp: loop identity (compute_dag_boulevard), per-occurrence facts, value identity (value_key), legality analysis.eval/ordered_schedule.hpp,cell_table.hpp,cell_table_builder.hpp: ordered schedule builder, explicit cell table with validator.eval/ordered_executor.hpp,cell_registry.hpp: table-driven executor (compute_cellreads operands through the table,apply_one_op, Sum/Scatter assembles, per-block scope guard for the backend's sparse-shape threshold).eval/backends/dryrun/: dry-run backend and metered walk/report.eval/ordered_dump.hpp: environment-gated diagnostics, documented in one place.Replaces #583 (same work, cleaned: whole-scope executor, cost-profile predictor, root-seed/cascade placement,
order_aware_recompute/node_level_placement, placement router and remat removed;CacheManager::store->store_and_access;stamp_lifetime_masksinsequant::eval). Kept for now: the batched forest descent (BatchScheduler::forest_descent).Requires TiledArray 58acda635 (first commit bumps the pin).
Node identity vs evaluation order (CSE comparator; f1b83fb, 4b96590)
Evaluation order stays exactly as the DP/binarizer emits it: cost, and peak memory in particular, is order-dependent, so the schedule has to be order-dependent and the tree is never permuted. Node identity, on the other hand, must not read that order. A Product node's children are therefore compared through a canonical view:
canonical_children(n)returns the two children ordered bycanonical_operand_cmp, which is O(1): a scalar operand sorts last (matching howbinarizebuilds ascalar * tensornode), then ascending node id (the operand-order-independentEvalExprhash), and a tie resolves to the emitted order. It reads only the two nodes' already-computed data.TreeNodeEqualityComparatorcomparescanonical_children(lhs)againstcanonical_children(rhs)pairwise, one recursion per child, so the comparison is linear, unconditionally; nothing is reordered and nothing is cached on the node. This replaces the earlier unordered either-pairing match (exponential in product depth).Resolving a node-id tie any other way was wrong: a fallback to a full equality comparison of the operand subtrees (as in f1b83fb) runs at every visit and makes the comparator T(n) = 4 T(n/2) = O(n^2) once hash-equal operands nest (a
t2 * t2self-contraction reaches it); 4b96590 deletes it. Returning 0 is right for the case that occurs: two operands that are one value have one node id, either order is canonical for them, and an ordered comparison of two swapped spellings succeeds either way. The only other way to reach the tie is a 64-bit collision between distinct values, where the cost is a missed fold (two cache entries instead of one), never a wrong one, since equality still compares both subtrees in full.The same reasoning applies to the value key the batched DAG scheduler derives from a node and its operands' keys: it combined the operand keys in emitted order and so did not fold swapped spellings even where the node cache did. Combining them in ascending order for Product nodes removes 10 duplicate builds from the water-20 aux+occ DAG schedule (1542 -> 1532 cold, 1286 -> 1276 warm). Evidence that the fold is right, since fewer builds alone proves nothing:
[w20-auxocc-walk], the strict dry-run walk over that schedule with lobound, range and fill-once checks in an assert-live build; a water-20 wet lossless run with this pin is queued as the end-to-end confirmation. The water-8 and he10 wet gates fold nothing and are regression gates only. The generated NEVPT2 ITF is byte-identical: the export path keys CSE offTreeNodeHasher+TreeNodeEqualityComparatorand never computes a DAG value key, and the canonical view folds a subset of what the unordered match folded, so export CSE lost nothing.