LTO support

[Krzmbrzl]

No description provided.

[Copilot]

Pull request overview

Adds project-wide optimization/LTO enablement via CMake and wires it into libraries, utilities, tests, and benchmarks, with accompanying CI and dependency updates.

Changes:

• Introduces target_set_optimization_flags() and applies it broadly to build targets (libraries, utilities, tests, benchmarks).
• Updates benchmarks to use a minimal MBPT operator registry and adds a “quick benchmarks” mode used in CI.
• Refreshes several pinned external dependency tags and bumps GitHub Actions versions.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
utilities/external-interface/CMakeLists.txt	Enables optimization/LTO flags for the external-interface utility target.
utilities/cost_analysis/CMakeLists.txt	Enables optimization/LTO flags for the cost_analysis utility target.
utilities/CMakeLists.txt	Enables optimization/LTO flags for all utilities built via the loop.
tests/unit/test_optimize.cpp	Adjusts lambdas and loop variable binding (contains a compile-breaking lambda change).
tests/unit/test_cache_manager.cpp	Explicitly ignores return values from store() to silence unused-result warnings.
tests/unit/CMakeLists.txt	Applies warning + optimization flags to unit test object libraries and the final unit test binary.
tests/integration/CMakeLists.txt	Applies optimization/LTO flags to integration test executables.
SeQuant/domain/mbpt/op.cpp	Avoids unused-variable warnings in assertion-only loops via [[maybe_unused]].
SeQuant/core/eval/eval.hpp	Initializes ResultPtr members explicitly to {}.
external/versions.cmake	Updates tracked tags/versions for several external dependencies.
CMakeLists.txt	Applies optimization/LTO flags to non-interface SeQuant module targets and SeQuant-bliss.
cmake/compiler.cmake	Adds target_set_optimization_flags() (contains LTO detection/enablement logic issues).
benchmarks/wick.cpp	Adds configurable input cap and uses make_minimal_registry() in benchmark context.
benchmarks/coupled_cluster.cpp	Adds configurable max rank and sets a minimal default MBPT context for the benchmark.
benchmarks/CMakeLists.txt	Applies warning/optimization flags and adds SEQUANT_QUICK_BENCHMARKS compile defs in Debug/quick mode.
.github/workflows/formatting_check.yml	Bumps checkout action major version.
.github/workflows/docs.yml	Bumps checkout/setup-python action major versions.
.github/workflows/cmake.yml	Bumps actions versions; enables quick benchmarks and adds a benchmark dry-run step.
.github/workflows/benchmark_compare.yml	Bumps actions/github-script, checkout, cache, and upload-artifact major versions.

Suppressed comments (2)

tests/unit/test_optimize.cpp:772

• This lambda no longer captures batch but still references it; since batch is a local variable this is a compile error.

        auto batch_fn = [](Index const&) -> std::size_t { return batch; };

tests/unit/test_optimize.cpp:814

• This lambda references the local variable batch without capturing it, which will fail to compile.

        auto batch_fn = [](Index const&) -> std::size_t { return batch; };

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ajay-mk]

@Krzmbrzl Could you update the CMake options table in docs?

[evaleev]

Code review: LTO support

Reviewed the full diff (CI action bumps, the new target_set_optimization_flags, benchmark OpRegistry fixes + "quick" mode, dependency bumps, warning fixes). 8 findings inline, ordered by severity below; the LTO flag-detection claims were verified empirically on Apple Clang 17 with a standalone CMake reproduction.

#	Where	Severity
1	cmake/compiler.cmake:95	medium/high
2	missed target_set_optimization_flags consumers	medium
3	cmake/compiler.cmake:67 (+ benchmarks/CMakeLists.txt:23)	medium
4	benchmarks/coupled_cluster.cpp:28 (maxRank)	medium
5	benchmarks/wick.cpp:131 (nMbptInputs)	low/medium
6	cmake/compiler.cmake:122	low
7	cmake/compiler.cmake:81,84-86	low
8	benchmarks/coupled_cluster.cpp:18	low

Verified as correct — no action needed

• SeQuant/core/eval/eval.hpp:560 — ResultPtr left/right = {} is safe: Frame remains an aggregate and all three construction sites (581, 656, 695) use designated initializers.
• tests/unit/test_optimize.cpp:730,772,814 — dropping the batch capture compiles; all three are std::size_t const batch = 1;, so the read is a constant expression and not an odr-use.
• --benchmark_dry_run=true exists in the tracked Google Benchmark (src/benchmark.cc:764 in v1.9.4, and SEQUANT_OLDEST_GOOGLEBENCHMARK_VERSION is 1.9.3), and build/benchmarks/sequant_benchmarks is the correct output path (no CMAKE_RUNTIME_OUTPUT_DIRECTORY is set).
• check_cxx_compiler_flag("-flto;-ffat-lto-objects" ...) does pass both flags — Internal/CheckCompilerFlag.cmake expands the list unquoted into CMAKE_REQUIRED_DEFINITIONS.
• make_minimal_registry() covers everything the fixed benchmarks need (t, h, f, g, plus L/R backing t::l/t::r), and spintrace genuinely needs no registry — so benchmarks/spintrace.cpp correctly went untouched.
• SEQUANT_QUICK_BENCHMARKS being option()-declared inside benchmarks/ is fine with the CI's unconditional -D...=ON; it only produces an "unused variable" notice in the valgrind/sanitize jobs where benchmarks are off.

Not verified

The external/versions.cmake tag bumps (utfcpp v4.1.1, CLI11 v2.7.2, spdlog v1.17.0, Catch2 v3.15.3, googlebenchmark v1.9.5, pybind11 v3.1.0) could not be checked — this review ran without network access, so git ls-remote against the upstream repos was unavailable. Someone should confirm those tags exist and that the SEQUANT_OLDEST_* floors are still accurate. Same for the actions/*@v7 / @v6 / @v9 bumps in the workflows.

[evaleev]

[3 — medium] The CMAKE_BUILD_TYPE STREQUAL "Debug" guard is a no-op under multi-config generators.

With Ninja Multi-Config, Xcode, or Visual Studio, CMAKE_BUILD_TYPE is empty, so this early return never fires and -flto=auto is applied unconditionally to every config, including Debug — slowing Debug builds substantially and degrading debuggability.

The identical guard at benchmarks/CMakeLists.txt:23 has the same problem in reverse: under a multi-config generator, SEQUANT_BENCH_MAX_CC_RANK / SEQUANT_BENCH_MAX_WICK_INPUTS are never defined for the Debug config, so the "don't let Debug benchmarks take forever" protection silently disappears.

Fix: use a $<CONFIG:Debug> generator expression for the flags (e.g. wrap the options in $<$<NOT:$<CONFIG:Debug>>:...>) rather than testing CMAKE_BUILD_TYPE at configure time.

[Krzmbrzl]

Correct though I don't think the added complexity is really worth the hassle. This can be dealt with once someone actually uses Windows to do SeQuant developing.

[evaleev]

Re-review at 192a150

Status of the 8 findings from the previous review (pinned to a1f1340). I re-read the current source rather than the replies, and verified the LTO behaviour with a standalone CMake repro that uses this branch's cmake/compiler.cmake verbatim (Apple clang 17.0.0).

#	Finding	Status
1	static/object gate used the wrong variable	Fixed (7460c7e)
2	consumers missing target_set_optimization_flags	Partially fixed (803ac19) — see below
3	CMAKE_BUILD_TYPE STREQUAL "Debug" is a no-op under multi-config	Not addressed — deliberate; I accept the call
4	un-capped maxRank = 10	Withdrawn — I was wrong
5	nMbptInputs not capped	Withdrawn as a timeout risk
6	forced SEQUANT_LTO=ON on a no-LTO toolchain	Not addressed — deliberate; I accept the call
7	result variables under generic/reserved names	Fixed (73ca17f) — one sub-claim of mine was overstated
8	global MBPT context mutated in a benchmark body	Fixed (e976a19, 192a150)

#1 verified empirically. Repro mirroring sequant_add_library (STATIC) plus an executable, both through target_set_optimization_flags:

-- Performing Test SEQUANT_FAT_LTO_FLAG_SUPPORTED - Failed
-- REPRO: lib_copts=o1-NOTFOUND exe_copts=-flto=auto exe_lopts=-flto=auto

The static library now gets no LTO flags where fat objects are unavailable, the executable still gets them, and the whole thing builds and links. The bitcode-only-archive hazard is gone.

#2 residual. python/CMakeLists.txt and tests/integration/eval/CMakeLists.txt were covered. doc/examples/CMakeLists.txt (line 48, target_set_warning_flags with no optimization call) is still the one consumer without it — now only a consistency gap, since #1's fix means the archives are only LTO'd where a plain link still works. Inline note anchored to tests/integration/eval/CMakeLists.txt since that file has no hunks in this diff.

#3 and #6 remain unaddressed by your deliberate choice, and I accept both. #3 is real but low-impact and the complexity isn't obviously worth it. On #6, "explicit user choice, explicit consequences" is a fair policy; the only thing I'd still suggest is that the CMake error a user actually sees never mentions SEQUANT_LTO.

Copilot's batch-lambda thread resolves in your favour. std::size_t const batch = 1; is a const integral with a constant initializer, so reading it inside the lambda is not an odr-use and no capture is required. Copilot is wrong; the code is correct as written.

#7, partial retraction of my own reasoning. The three cache variables are properly prefixed now. My sub-claim about CMAKE_SUPPORTS_COMPILER_LTO was overstated: it is a non-cached function-local that check_ipo_supported overwrites on every call, so the parent-shadowing scenario I described cannot apply to it. Naming nit only.

Withdrawals for #4 and #5 are posted as replies on their original threads.

Four new findings on the new commits below, all low or medium-low. One of them (python/CMakeLists.txt:16) exists only because of my own bad advice in the previous review — pybind11 already applies LTO to that target, which I should have checked before asking for the call to be added. Sorry for the churn.

[evaleev]

[A — medium-low] LTO link options are enabled by a probe that never links.

a1f1340 added set(CMAKE_TRY_COMPILE_TARGET_TYPE "STATIC_LIBRARY") at line 83, which makes all three check_cxx_compiler_flag probes compile-only — the commit message states the assumption outright ("under the assumption that if the compiler supports the flag, we will encounter a linker that can handle LTO").

This line then enables LTO on SEQUANT_LTO_FLAG_SUPPORTED OR CMAKE_SUPPORTS_COMPILER_LTO, and lines 108/112 put -flto=auto / -flto into target_link_options. So the decision to hand LTO flags to the linker is made by a probe that never invoked the linker, and the one check that does link (check_ipo_supported, line 81) is OR'd away.

Scenario: GCC or Clang paired with a GNU ld that has no LTO plugin — a GCC built against plugin-less binutils, or Clang defaulting to bfd ld with no LLVMgold.so. -flto compiles fine, so the probe passes, and then every SeQuant executable / shared / module target fails to link in Release with plugin needed to handle lto object or a wall of undefined symbols. Nothing in the configure output hints at the cause.

Fix: AND on CMAKE_SUPPORTS_COMPILER_LTO here, or restore an EXECUTABLE try-compile target type for the plain -flto probe (keeping STATIC_LIBRARY only for the fat-objects probe, where no link is involved anyway).

[Krzmbrzl]

GNU ld accepts GNU-style LTO options for compatibility for pretty much exactly this reason. From the output of ld --help:

-flto Ignored for GCC LTO option compatibility

We'll keep as-is and can deal with any issues should they appear. These days, I don't think there are that many non-LTO capable linkers in-use anymore.