ContractReduce: fall back to value-returning conj when a tile has no conj_to - #586
Conversation
…conj_to
conj and conj_to are independent tile-interface customization points, and the
default conj_to is SFINAE-constrained on a conj_to() member, so a tile that
implements only the value-returning conj is a legal partial implementation
that could not be used in a conjugated contraction. The two CPOs have exactly
one call site each in the library -- the same function, ContractReduce's
ComplexConjugate finalization -- picking between them on whether a result
permutation is present, so such a tile compiled
c("k,i") = conj(a("i,j") * b("j,k")); // permuted -> conj
c("i,k") = conj(a("i,j") * b("j,k")); // unpermuted -> conj_to, hard error
- TiledArray::has_conj_to_v<T...>: whether the ADL call conj_to(args...) is
viable, declared next to the CPOs it describes.
- detail::conj_finalize(temp, factor...): in place through conj_to when that
is viable, else through conj. Both unpermuted branches of the finalization
route through it; the permuted ones already used conj.
The detection is on the ADL CALL, not on a conj_to() member. btas::Tensor has
no member -- only free functions in namespace btas -- so a member-based test
would report false for it and silently move it from in-place conjugation to an
allocate/copy/free per tile, a performance regression no existing test would
catch. tile_op_contract_reduce.cpp static_asserts that distinction.
No behavior change for any tile in the tree: Tensor, TensorInterface,
ArenaTensor, Tile, btas::Tensor and the nested Tensor<ArenaTensor<...>> /
Tensor<Tensor<...>> forms all satisfy has_conj_to_v and keep the in-place path.
Tests: has_conj_to_v over those types with and without a scale (plus the BTAS
static_assert); the Tensor path conjugating its argument in place; and the
regression itself -- ContractReduce<ConjOnlyTile, ..., ComplexConjugate<void>>
and <..., ComplexConjugate<double>> finalizations run for a tile carrying the
four value-returning conj overloads and no conj_to.
perm() is a runtime value, so both branches of the ComplexConjugate finalization are instantiated for every tile used there: the permuted one always needs the value-returning conj(result, perm) (or conj(result, factor, perm)) whether or not a permutation is ever applied, exactly as the primary template's finalization unconditionally instantiates Permute<Result, Result>. Only conj_to is optional, via the fallback added in the previous commit. A tile missing one of those overloads currently fails inside tile_interface.h's default conj with "too many arguments to function call", which names neither the tile nor the requirement. Record the contract where it bites, and record why it is prose rather than a static_assert: the four conj CPOs are constrained only on Perm being a permutation and have a DEDUCED return type, so decltype(conj(arg, perm)) has to instantiate the body -- detecting them hard-errors instead of yielding false, unlike conj_to, whose CPO is constrained on the member the way neg_to's is. Making conj detectable means constraining those four overloads, a change to a public header's overload set that wants its own PR. Documentation only; no change to generated code.
|
Added the tile-requirement contract as documentation on the finalization (6955f11). I tried it first as a
So the requirement is prose for now, and the note says what it would take to make it enforceable: constrain the four Verified: the sweep over every TU that instantiates this finalization — |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved blocking issues were identified, and regression coverage is included.
Pull request overview
Adds ADL-based conj_to detection and falls back to value-returning conj during ContractReduce finalization.
Changes:
- Adds
has_conj_to_v. - Selects in-place or fallback conjugation.
- Adds regression and detection tests.
File summaries
| File | Summary |
|---|---|
tests/tile_op_contract_reduce.cpp |
Tests detection and fallback behavior. |
src/TiledArray/tile_op/tile_interface.h |
Adds ADL-based conj_to detection. |
src/TiledArray/tile_op/contract_reduce.h |
Implements in-place or value-returning finalization. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Closes #585.
Problem
TiledArray::conjandTiledArray::conj_toare independent ADL customization points, and the defaultconj_tointile_op/tile_interface.his SFINAE-constrained on aconj_to()member. A custom tile that implements only the value-returningconjis a legal partial implementation of the tile interface — the patternAGENTS.mdblesses, withtests/sparse_tile.h'sEigenSparseTileomittingsubtas precedent — but it could not be used in a conjugated contraction.The two CPOs have exactly one call site each in the whole library, and it is the same function:
ContractReduce'sComplexConjugatefinalization, choosing between them on whether a result permutation is present. So for such a tile:On
master(84cc198) instantiating that finalization iscontract_reduce.h:574: no matching function for call to 'conj_to', and:581for the scaled specialization.Unary
conj(a("i,j"))was never affected — it is aScalexpression with aComplexConjugatefactor and goes throughscale/scale_to.Change
TiledArray::has_conj_to_v<T...>intile_op/tile_interface.h, next to the CPOs it describes: whether the ADL callconj_to(args...)is viable.detail::conj_finalize(temp, factor...)incontract_reduce.h: in place throughconj_towhen that is viable, otherwise through the value-returningconj. Both unpermuted branches of the finalization route through it; the permuted branches already usedconjand are untouched.The detection is on the ADL call, not on a
conj_to()member.btas::Tensorhas no such member — only free functions in namespacebtas— sodetail::has_member_function_conj_to_anyreturn_vreports false for it, and a member-based test would silently demote BTAS from in-place conjugation to an allocate/copy/free per tile. That is a performance regression that would pass every existing test, sotile_op_contract_reduce.cppcarries astatic_assertpinning the distinction.No behavior change for any tile in the tree:
Tensor,TensorInterface,ArenaTensor,Tile,btas::Tensorand the nestedTensor<ArenaTensor<...>>/Tensor<Tensor<...>>forms all satisfyhas_conj_to_vand keep the in-place path.Tests
Three cases in
tile_op_contract_reduce_suite:conj_to_detection—has_conj_to_vis true forTensor<complex>,btas::Tensor<complex>,Tensor<ArenaTensor<complex>>andTensor<Tensor<complex>>, with and without a scale; false for the fallback tile. Plus thestatic_assertrecording that BTAS has noconj_tomember.conj_finalize_in_place_when_supported— theTensorpath conjugates the argument itself, i.e. no copy is made.conj_finalization_without_conj_to— the regression: buildsContractReduce<ConjOnlyTile, …, ComplexConjugate<void>>and<…, ComplexConjugate<double>>and runs the finalization.ConjOnlyTilesupplies the four value-returningconjoverloads and noconj_to, member or free.tile_op_contract_reduce24/24 andtot_expressions(which exercises this finalization heavily on owning, arena and BTAS cells) pass under ASan. Verified that the regression bites: the sameContractReduceinstantiation written against pre-existing API fails onmasterwith theconj_toerror above and compiles here.