Tensor<ArenaTensor>: in-place binary ops must not drop populated cells into null destinations - #575
Open
evaleev wants to merge 4 commits into
Open
Tensor<ArenaTensor>: in-place binary ops must not drop populated cells into null destinations#575evaleev wants to merge 4 commits into
evaleev wants to merge 4 commits into
Conversation
…l when a destination cell is null; arena kernels throw instead of dropping data
An `ArenaTensor` inner cell is a non-owning view: a null cell has no storage
to write into and cannot allocate. The free in-place kernels `add_to` /
`subt_to` / `mult_to` therefore opened with `if (!dst || !src) return;`, so a
null destination cell silently discarded a populated source cell. Reached
per-cell from `Tensor::add_to` -> `inplace_binary` -> `l += r`, this lost an
entire addend whenever two ToT operands disagreed on which inner cells are
populated -- a routine case, since the same outer tile can aggregate several
pairs with some screened to null.
`Tensor::{add,subt,mult,axpy}_to` (scaled variants included) now detect an
ordinal with a null left cell and a populated right cell and route through
the value-returning kernel, which builds a fresh slab with union sparsity.
The detection is compiled out unless both operands' cells are views, so the
plain-inner and flat paths are untouched.
The free per-cell kernels now throw rather than no-op in that case, so any
remaining path that drops data is loud instead of silent. `mult_to` also
gains the mirror fix: a null *source* is an implicit zero, so it must zero
the destination rather than leave it unchanged.
…n in ToT Tensor::add on an empty left `ArrayEvalImpl::is_consumable()` is true whenever the operand carries a permutation, so `BinaryWrapper` rewrote an out-of-place `a(perm) + b` as `a(perm).add_to(b)`. For `Tensor<ArenaTensor>` that turned a correct union-sparsity add into an in-place one whose destination cells may be null. The previous commit makes that fall back safely, but paying for a re-homed copy on every such add is the wrong shape: consuming a tile is only sound when every destination cell has storage to write into, which a view cell that is null does not. Specialize `is_consumable_tile<Tensor<T>>` to false for view cells so these expressions stay on the value-returning kernel. Also add the `return` missing from `Tensor::add`'s empty-left early exit, so a plain-inner ToT `add` on a default-constructed left yields the right-hand operand instead of falling through to the binary kernel with mismatched outer volumes (an empty result, or a TA_ASSERT trip when assertions are on). The arena overload already had it.
…ero, not an error The previous commit made the free `mult_to` throw on a null destination cell with a populated source, matching `add_to` / `subt_to`. That is wrong for multiplication: the product is zero, a null cell is exactly how a sparse ToT spells zero, and nothing is lost. Screened-pair Hadamard products routinely produce that shape, so the exception would break real workloads. Restore the silent no-op there (the destination stays null). The rest of the null-cell policy stands: `add_to`, `subt_to` and `axpy_to` still throw on a null destination, where returning would genuinely drop the source, and `mult_to` still zeroes the destination when the *source* is null (`dst *= 0` is not a no-op). The header now spells out the asymmetry and why. `Tensor::mult_to` keeps its tensor-of-view fallback, so an in-place and an out-of-place mult agree on sparsity as well as on values; the test checks the numeric result and accepts either spelling of zero.
…(fixes complex tile instantiation) The null-destination fallback added to the in-place binary ops was a plain runtime `if`, so its body was instantiated for *every* `Tensor`, not only the tensor-of-view ones it can ever fire for. The body calls the value-returning kernels, which need not be well-formed for an operand/scalar mix the in-place path does support: `Tensor<std::complex<double>>::subt_to(right, int)` is fine elementwise -- `(l -= r) *= 2` resolves to `complex<double>::operator*=(const double&)` -- but the `subt(right, int)` it would fall back to is not, since binary `operator*(const complex<_Tp>&, const _Tp&)` cannot deduce `_Tp` from a `complex<double>` / `int` pair. That broke downstream builds instantiating TA for complex tiles. Gate every fallback -- `add_to`, `subt_to`, `mult_to`, `axpy_to`, scaled and unscaled -- on a new `binary_needs_view_cell_fallback_v<Right>` used as an `if constexpr` condition, so nothing is instantiated for a non-view tensor. `inplace_binary_drops_cells` consults the same constant, keeping one source of truth for the gate. Adds a compile-only regression instantiating the complex-tile in-place ops with an `int` factor, plus static_asserts pinning the two traits the gate is built from, so a trait change cannot silently delete the fallback instead.
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, compile-time guarded, and backed by targeted regressions that exercise the previously silent data-loss paths.
Pull request overview
Fixes a correctness bug for tensor-of-view nested tiles (Tensor<ArenaTensor<T>>) where in-place binary ops could silently drop populated right-hand inner cells when the destination inner cell was null, especially when expression evaluation rewrote out-of-place adds into in-place ops due to “consumable” tiles.
Changes:
- Add an
if constexpr-guarded fallback inTensor::add_to/subt_to/mult_to/axpy_toto route through value-returning (union-sparsity) kernels when an in-place update would drop populated view-cells. - Mark
Tensor<T>as non-consumable whenTis a tensor view to prevent expression-level rewrites into in-place updates for view-cell tensors. - Tighten arena view-cell in-place kernels to throw on null-destination/populated-source for additive ops (and make
mult_totreat null source as multiplicative zero).
File summaries
| File | Description |
|---|---|
| tests/arena_tot_trivial.cpp | Adds regression tests covering null-destination view cells, expression-level permuted add rewrite, and compile-only complex-tile instantiation cases. |
| src/TiledArray/tensor/type_traits.h | Makes Tensor<T> non-consumable when T is a tensor view, preventing unsafe in-place consumption in expression eval. |
| src/TiledArray/tensor/tensor.h | Adds compile-time-gated detection + fallback to value-returning kernels for ToT view-cell in-place binary ops; fixes missing return for empty-left add. |
| src/TiledArray/tensor/arena_tensor.h | Updates arena per-cell _to kernels to throw on null destination for additive ops and to zero destination on null source for mult_to. |
Review details
- Files reviewed: 4/4 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.
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.
Nested tiles
Tensor<ArenaTensor<T>>lost data in in-place addition when a destination inner cell was null while the source cell was populated: the arenaadd_tokernel returned silently on a null destination (anArenaTensoris a non-owning view and cannot adopt the source), and out-of-placea(l) + b(r)was routed into that in-place path byAdd::evalwhenever an operand carried a permutation (ArrayEvalImpl::is_consumable()), sox = a + bcould annihilatebwhilex += bdid not. Found through a NaN residual in a downstream application (an all-empty nested intermediate added to a populated one).Changes:
Tensor::add_to/subt_to/mult_to/axpy_to(scaled and unscaled) for tensor-of-view value types fall back to the value-returning union kernel when any destination cell is null and the source cell is not; guarded withif constexprso non-view tensors (including complex tiles with integral factors) are unaffected.is_consumable_tile<Tensor<T>>is false whenTis a tensor view, soBinaryWrappernever converts an out-of-place add over arena tiles into an in-place one.add_to/subt_to/axpy_tothrowTiledArray::Exceptionon a null destination with a populated source instead of dropping data;mult_totreats a null destination as zero (no error) and now zeroes the destination on a null source (previously left unchanged).returnin the tensor-of-tensorTensor::addwhen the left operand is empty (fell through into the kernel and produced an empty result).tests/arena_tot_trivial.cpp: tile-leveladd_to/subt_to/mult_to/axpy_towith all-null and mixed-null cells, the kernel null-destination policy, an expression-level permuted add over an all-null arena array, an empty-left plain tensor-of-tensor add, and a complex-tile instantiation regression. Each data-loss case failed before the fix.