ToT: conj() on nested-tile contractions; real-plain x complex-ToT products via one real gemm - #574
ToT: conj() on nested-tile contractions; real-plain x complex-ToT products via one real gemm#574kshitij-05 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new interleaved real-GEMM fast path relies on reinterpret_cast assumptions about std::complex<T> representation and needs compile-time layout/alignment guards to avoid potential UB.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the ToT (tensor-of-tensor) contraction machinery to correctly support conj() around nested-tile contractions and adds a fast path for complex-ToT × real-plain tensor products by running a single real GEMM on an interleaved (re,im) view.
Changes:
- Introduces
detail::elem_factor<Numeric>(factor)to extract the per-element contraction multiplier (numeric factor, or1forComplexConjugate<...>factors) and wires it into ToT per-cell ops andTensor::gemmalpha handling. - Refactors
ContractReduceto share nested-tile accumulation and partial-result reduction logic across the primary template andComplexConjugate<...>specializations (removing the previous nested-tileabort()stub). - Adds a mixed-type ToT × real-plain product optimization by viewing complex slabs as interleaved real matrices (doubling inner extent) and adds a focused
tot_conjtest suite covering these cases.
File summaries
| File | Description |
|---|---|
| tests/tot_expressions.cpp | Adds tot_conj tests covering unary conj, conj around products, inner+outer contractions, and complex-ToT × real-plain products. |
| src/TiledArray/tile_op/contract_reduce.h | Shares nested accumulation + reduction logic and enables nested ToT accumulation for ComplexConjugate<...> factors. |
| src/TiledArray/tensor/tensor.h | Uses elem_factor for GEMM alpha and adds the interleaved real-GEMM fast path for complex-ToT × real-plain tensor scale products. |
| src/TiledArray/tensor/complex.h | Adds is_complex_conjugate_v and elem_factor helper for contraction-factor handling. |
| src/TiledArray/expressions/cont_engine.h | Switches ToT per-cell contraction factor application to elem_factor so ComplexConjugate<...> factors no longer require element-type casts. |
Review details
Suppressed comments (1)
src/TiledArray/tensor/tensor.h:3531
- Same interleaved real-GEMM reinterpret_cast is used in the mirrored T * ToT scale path. Add the same compile-time layout/alignment guard for std::complex here as well, so the reinterpret_cast to Ur* is only used when the representation assumptions hold.
constexpr bool same_type = std::is_same_v<Ur, Real>;
constexpr bool interleaved =
!same_type && std::is_same_v<std::complex<Ur>, Real>;
constexpr integer cw = interleaved ? 2 : 1; // reals per inner element
if constexpr (same_type || interleaved) {
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f602d22 to
d901e77
Compare
15b2f59 to
4fd5dca
Compare
3cf175c to
8ea2fa9
Compare
…ducts via one real gemm conj(A * B) on tensor-of-tensor operands did not compile: ContEngine's per-cell multiply-add ops static_cast the ComplexConjugate<void> contraction factor to the element type, Tensor::gemm forwarded it as the BLAS alpha, the ContractReduce<..., ComplexConjugate<...>> specializations named their result type through a value-returning gemm nested tiles do not have, and their nested-tile accumulate was an abort() stub. - detail::elem_factor<Numeric>(factor): the per-element multiplier for a contraction factor -- the factor for numeric factors, 1 for ComplexConjugate<...> (conjugation and scale are applied to the finished result by ContractReduce's finalization, as for non-nested tiles); used by the ToT per-cell ops in ContEngine and by Tensor::gemm's alpha - ContractReduce ComplexConjugate specializations: result_type is Result; the nested-tile accumulate and the arena-aware partial-result reduce are shared with the primary template (ContractReduceBase::accumulate_nested / reduce_results) - ToT x real plain-tensor products: when the plain element type is the real part of the inner element type, the complex slabs are viewed as real matrices with the inner extent doubled (re,im interleaved) and the strided GEMM fast path runs in real arithmetic with alpha = beta = 1 - tests: tot_conj suite in tot_expressions.cpp -- conj(a), permuted conj(a), conj(a)*b, a*conj(b), conj(a*b), conj(a)*b with an inner contraction, and the mixed-type ToT x real-plain product, against explicit references
…aved gemm relies on The interleaved real-gemm path reinterprets a std::complex<Vr>/<Ur> slab as a real matrix with twice the column count. The standard guarantees that layout (array-oriented access, [complex.numbers.general]/4); pin it with a static_assert at both interleaved sites so an exotic ABI fails to compile instead of silently producing garbage. Also reflow one over-long line in the tot_expressions make_tot_1 helper (clang-format).
evaleev
left a comment
There was a problem hiding this comment.
Reviewed f399b490f..4fd5dcaa8 on top of #573. I built each changed header into probe programs (linked against a debug libtiledarray) and ran them, so the findings below are confirmed by execution rather than by reading.
Copilot's comment: already addressed
Copilot's single finding (guard the reinterpret_cast<Vr*> on std::complex<Vr> layout) is answered by 4fd5dcaa8, which adds the static_assert at both sites — including the mirrored t x ToT path at tensor.h:3729 that Copilot listed as a suppressed comment. Nothing left to do there.
One nit on the guard: alignof(Real) == alignof(Vr) is stricter than the cast needs (over-alignment of complex<double> is harmless), and because it is a static_assert rather than a term in the interleaved predicate, an ABI that over-aligns complex would make TA fail to compile rather than fall back to the AXPY loop. Folding the condition into constexpr bool interleaved degrades gracefully instead.
Blocking: two silent wrong answers, both new
Both cases were compile errors before this PR, so the PR turns hard failures into quiet incorrect results.
1. A ComplexConjugate<Scalar> prefactor is dropped on ToT contractions
complex.h:310 + cont_engine.h:435,498.
elem_factor returns Numeric(1) for any ComplexConjugate<...>, deferring conjugation and scale to ContractReduce's finalization. But the nested branch builds the outer op as op_type(..., scalar_type(1), ...), so finalization runs conj_to(temp, factor().factor()) with factor() == 1. The magnitude is applied nowhere.
On Tensor<Tensor<complex<double>>>:
2.0 * conj(a("i,j;a") * b("j,k;b")) got (-0.5,-0) ref (-1,-0) # factor 2 gone
-conj(a("i,j;a") * b("j,k;b")) got (-0.5,-0) ref ( 0.5, 0) # sign gone
-conj(A*B) is an ordinary expression, not a corner case. Either propagate factor_ into the op's scalar_type for the nested branch, or have elem_factor return the ComplexConjugate<Scalar> magnitude.
2. conj(ToT * complex-plain) double-conjugates the plain operand
cont_engine.h:2006.
fallback_op bakes the raw factor_ in per element (right * factor => conj(right) for ComplexConjugate<void>), and finalization then conjugates the whole result again. The fused arena lambda is factor-free and correct, so the two paths now disagree. Reproduced with an inner permutation, which makes make_contraction_arena_plan return nullopt and select fallback_op:
c("i,k;b,a") = conj(a("i,j;a,b") * t("j,k")); // t complex plain
got (-0.5,-7.1) ref (-3.5,-5.9)
(-0.5,-7.1) is exactly sum_j conj(a_j) * t_j — t never gets conjugated. make_fused_hadamard_scaled_lambda(this->factor_) at cont_engine.h:1257/1899 has the same shape and was also left on the raw factor.
The stated goal is not reached for the arena tile type
conj(a("i,j;x") * b("j,k;y")) still does not compile for Tensor<ArenaTensor<complex<double>>> — the production tile type this series exists to optimize (per #573's commit message: MPQC Kramers PNS-CCD). The static_cast<T>(factor) pattern this PR replaced with elem_factor in Tensor::gemm survives in the arena mirror at arena_tensor.h:599, reached via arena_einsum.h:1110 <- fused_contraction_inplace <- init_inner_tile_op. Everything else on arena tiles works:
| expression | owning Tensor<Tensor<cplx>> |
Tensor<ArenaTensor<cplx>> |
|---|---|---|
conj(a) |
works | works |
conj(a) * b, a * conj(b) |
works | works |
conj(a * b) |
works | compile error |
Not a regression (it fails on master too), but it means the first half of the PR title holds only for owning tiles.
Test coverage gaps
- The interleaved real-GEMM kernel ships untested. Both blocks gate on
is_tensor_view_v<U> && is_tensor_view_v<value_type>, butconj_test_paramsuses owningTensor<Tensor<complex<double>>>(the file comment says so).tot_times_real_plaintherefore validates only the per-cell AXPY fallback. I wrote the missing case — arena complex ToT x real plain, checked against an owning reference — and it is correct: 16 strided GEMM runs, 0 fallbacks, max error 0 (verified viaTA_GEMM_TIMING=1). The kernel is fine; it just has no test, and that case is worth adding. - No test covers a scaled or negated
conjon a ToT (finding 1), orconjcombined with view/arena inner cells —reduce_resultsnow routes those througharena_tot_add_tofor theComplexConjugatespecializations, which is new and unexercised.
Minor
tensor.h:3552,3729: thereinterpret_cast<const Vr*>(lc0[0].data())is applied in thesame_typebranch too, andinterleavedis decided fromVand the result's inner scalar — the left operand's inner scalar (typename U::value_type) is never checked. I could not construct a reachable mismatch (the mixed real-ToT -> complex-result assignment does not compile), so this is hardening rather than a live bug: addingstd::is_same_v<typename U::value_type, Real>to the predicate restores what the natural-typed pointer used to guarantee.contract_reduce.h:491,620:result_typechanged fromdecltype(gemm(declval<Left>(), declval<Right>(), 1, helper))toResult. For homogeneous cases these coincide and the change looks right (it aligns withContractReduceBase), but it is a public-header behavior change worth calling out per the repo's PR-review guidance.contract_reduce.h:578,707:using TiledArray::empty;now appears twice (function scope +elseblock). Harmless; drop the inner one.- Genuinely good: sharing
accumulate_nested/reduce_resultsacross the primary template and both specializations removes theabort()stub and keeps the three in sync by construction.
Verdict
Findings 1 and 2 need fixing before merge — both produce wrong numbers with no diagnostic on expressions a user would naturally write. The interleaved GEMM is correct but needs the arena test that actually exercises it.
evaleev
left a comment
There was a problem hiding this comment.
Requesting changes on the two blockers from the detailed review above. Both were compile errors before this PR and are silent wrong answers after it, on expressions a user would naturally write:
-
ComplexConjugate<Scalar>prefactor dropped on ToT contractions (complex.h:310,cont_engine.h:435,498) —elem_factorreturns 1 for anyComplexConjugate<...>, but the nested branch builds the op withscalar_type(1), so finalization conjugates with factor 1 and the magnitude is applied nowhere.-conj(a("i,j;a") * b("j,k;b"))returns the unnegated result. -
conj(ToT * complex-plain)double-conjugates the plain operand (cont_engine.h:2006) —fallback_opapplies the rawfactor_per element and finalization conjugates the whole result. With an inner permutation (arena plan bails tonullopt) the result issum_j conj(a_j) * t_jinstead ofconj(sum_j a_j * t_j). Same shape atcont_engine.h:1257/1899.
The interleaved real-GEMM path is correct as written — it just needs the arena test that actually exercises it, since the added tot_times_real_plain case uses owning tiles and never enters that code.
…op's finalization A conj(A*B) factor cannot be applied per cell (conj does not distribute into a sum of products), so ContEngine now hands every consumer one of two things: elem_scale(), the numeric per-cell multiplier (the factor itself, 1 for a ComplexConjugate<...>), and outer_factor(), the factor of the outer tile op (1 for a numeric factor, whose scale the cells absorb; the ComplexConjugate<...> itself otherwise, so its finalization conjugates AND scales the finished tile). No site reads factor_ directly any more. Before, six of sixteen consumers used elem_factor() and the rest the raw factor, so S*conj(A*B) and -conj(A*B) dropped the scale/sign, the inner-Hadamard form double-conjugated, a complex plain operand came out un-conjugated, and every arena-cell conj(A*B) failed to compile. - ContractReduceBase: a ComplexConjugate alpha may accompany a per-cell op (the "non-unit alpha must be absorbed" assertion applies to numeric factors only). - BatchedContractReduce: the finalization forwards to the wrapped op's, so batched (fused-mode) conj(A*B) is conjugated like the unbatched one (this was wrong for plain tiles too). - ArenaTensor: operator*= overloads for ComplexConjugate factors (in-place, via the free scale_to kernel); the gemm overload rejects a ComplexConjugate factor at compile time. - Tensor::scale_to: a ComplexConjugate factor on owning nested cells conjugates each cell in place instead of `cell = conj(cell) * S` (allocation + copy per cell); real elements skip the conjugation; arena cells go through the free kernel for every factor type. - Tensor::gemm: a ComplexConjugate alpha is a static_assert again (it was silently mapped to 1). - ContractReduce: the two ComplexConjugate specializations (120 lines apart in two finalization lines) are one specialization on ComplexConjugate<S> with an if constexpr on void. - complex.h: the mixed-scalar operator* templates are declared before the ComplexConjugate operators, so an integer-literal scale (2 * conj(a*b)) compiles. - [scale-timing]: the interleaved real-gemm path counts FLOPs in element multiply-adds, the unit of the other paths. - tests (tot_conj): scaled / negated / integer-scaled conj of a product, scaled conj of an inner contraction and of an inner Hadamard, conj of a ToT x complex plain product (with an inner permutation), multi-tile scaled conj, fused-mode conj (ToT and plain), arena-cell conj forms, and the arena ToT x real plain products that exercise the interleaved real-gemm fast path.
4fd5dca to
0a8dea7
Compare
The multi-tile check gathered tiles through the local-tile iterators, so under two MPI ranks the other rank's tiles stayed empty and the Debug assertion in Tensor::operator() fired (CI run-np-2; run-np-1 passed).
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level contraction/GEMM paths and factor handling across multiple ToT execution modes, so it warrants final human review despite strong test additions.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| /// cell -- conj does not distribute into a sum of products -- so the | ||
| /// per-cell ops run with multiplier 1 and the outer op's finalization | ||
| /// conjugates AND scales the finished tile (ContractReduce's ComplexConjugate | ||
| /// specializations). Every consumer of the factor goes through these two | ||
| /// accessors; none may read factor_ directly. |
…ote scoped to ToT-aware ops - Tensor::gemm (tot_x_t and t_x_tot): the std::complex<Vr> == Vr[2] layout requirement moves from a static_assert into the `interleaved` predicate (detail::is_interleaved_real_view_v, shared by both blocks; alignment needs only >=), so an ABI that breaks it falls back to the per-cell loop instead of failing to compile. The ToT operand's inner scalar must match the result's for either GEMM branch (both slabs are viewed through the same scalar pointer). - ContEngine: the factor comment states what the code does -- plain-tensor contractions hand factor_ to ContractReduce (GEMM alpha, or the ComplexConjugate finalization) and the shape GEMMs take it too; only the ToT-aware ops go through elem_scale()/outer_factor().
Follow-up to #573 (merged); rebased onto
master.Problem
conj()on a ToT contraction does not compile.conj(A * B)carries aComplexConjugate<void>contraction factor; the ToT per-cell multiply-add ops inContEnginestatic_castthat factor to the element type,Tensor::gemmforwards it as the BLAS alpha, and theContractReduce<…, ComplexConjugate<…>>specializations name their result type through a value-returninggemmthat nested tiles do not have.A("i,j;a") * t("j,k"),Tensor<Tensor<complex<double>>>×Tensor<double>) skips the ToT × plain-tensor GEMM fast path, which requires identical element types, and runs the per-cell AXPY loop.Changes
detail::elem_factor<Numeric>(factor): the per-element multiplier for a contraction factor — the factor for numeric factors,1forComplexConjugate<…>(conjugation and scale are applied to the finished result byContractReduce's finalization, as for non-nested tiles). Used by the ToT per-cell ops inContEngineand byTensor::gemm's alpha.ContractReduce<…, ComplexConjugate<void>>/<…, ComplexConjugate<Scalar>>:result_typeisResult, as in the primary template, and the nested-tile accumulate (which was anabort(); // not yet implementedstub) and the partial-result reduce are shared with the primary template throughContractReduceBase::accumulate_nested/reduce_results.Tensor::gemm's interleaved real-GEMM path takes its layout requirement (std::complex<Vr>is two contiguousVr,alignofat leastVr's) and the ToT operand's inner-scalar match as terms of theinterleavedpredicate (detail::is_interleaved_real_view_v, shared by thetot_x_tandt_x_totblocks), so an ABI that breaks the layout falls back to the per-cell loop instead of failing to compile or reinterpreting into UB.ContractReduce<…, ComplexConjugate<…>>::result_typeisResult, as in the primary template, instead of the value type of agemm(left, right, 1, helper)call (identical for homogeneous tiles; public-header change).ContEngine(elem_scale()for the per-cell ops,outer_factor()for the outer tile op) instead of per consumer, soS*conj(A*B),-conj(A*B), the inner-Hadamard and complex-plain-operand forms, and batched (fused-mode)conj(A*B)all apply the conjugation and scale exactly once; arena-cell (Tensor<ArenaTensor<complex>>)conj(A*B)compiles (ArenaTensorgainsoperator*=forComplexConjugatefactors); nested-cell conjugation in the finalization is in place;Tensor::gemmrejects aComplexConjugatealpha at compile time again; an integer-literal scale (2 * conj(a*b)) compiles.tot_conjsuite intests/tot_expressions.cpp, real and complex rows):conj(a), permutedconj(a),conj(a) * b,a * conj(b),conj(a * b)(outer contraction with inner outer product),conj(a) * bwith an inner contraction, and the mixed-type ToT × real-plain product, all against explicit references.Testing
tot_expressions(incl. the extendedtot_conj: scaled/negated/integer-scaled conj, inner contraction and inner Hadamard, complex plain operand, multi-tile, fused mode, arena cells, interleaved real-gemm path),einsum,arena_strided_gemm:tot_expressions362/362 cases (3486 assertions;tot_conj22 cases / 808 assertions),einsum59/59 (516),arena_strided_gemm60/60 (1869), all against currentmaster(post-#573, #581).Not addressed
tot_x_tandt_x_totblocks ofTensor::gemm.