JDBetteridge/compression - #2980
Open
JDBetteridge wants to merge 57 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
=======================================
Coverage 83.68% 83.68%
=======================================
Files 257 257
Lines 54711 54711
Branches 4686 4686
=======================================
Hits 45785 45785
Misses 8115 8115
Partials 811 811
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
EdCaunt
approved these changes
Jul 28, 2026
EdCaunt
left a comment
EdCaunt
left a comment
Contributor
There was a problem hiding this comment.
Seems reasonable modulo ruff fixes
mloubout
force-pushed
the
JDBetteridge/amd_compression
branch
from
August 25, 2026 13:39
bed8a9f to
3151a57
Compare
JDBetteridge
commented
Aug 25, 2026
When a host-device data transfer is linearized, its array section size is emitted as a product of the Function's per-dimension sizes, e.g. `copyin(u[0:u_vec->size[0]*u_vec->size[1]*u_vec->size[2]*u_vec->size[3]])`. The `size[i]` fields are 32-bit C ints, so for a Function with more than ~2**31 elements the product overflows `int` before it is used as the transfer bound, yielding a bogus size and a corrupt/failed device transfer. Cast each factor of the product to a 64-bit integer so the multiplication is carried out in 64-bit arithmetic. Casting the whole product would be too late (the overflow would already have occurred), so each factor is cast individually. Non-product bounds (a single size, an offset, a constant) cannot overflow and are left untouched, as are non-transfer expressions. Fixes #2777
Address review: replace the ad-hoc _avoid_overflow helper with the existing as_long. as_long only substituted plain Symbols (retrieve_symbols), so it was a no-op on the IndexedPointer size factors (vec->size[i]) of a linearized transfer bound; extend it to retrieve_terminals so Indexed/IndexedPointer leaves are cast too. Keep the cast scoped to Mul products in PragmaTransfer so non-linearized multi-dimensional sections are not needlessly upcast.
…nt directly Per review: as_long already walks the expression args, so the cast() helper and its is_Mul check are unnecessary. Apply as_long to the section extent directly. Output is unchanged: the start bound is always 0/an offset (left as-is) and the extent is the size product that as_long promotes to 64-bit.
…tebook Applying as_long unconditionally also rewrote plain per-dimension extents (IndexedPointer leaves), churning every non-linearized transfer pragma and breaking the openacc codegen tests and the 01_gpu notebook. Only a product of 32-bit sizes can overflow int, so restrict the promotion to Mul bounds and update the notebook's reference output for the linearized transfer clauses that legitimately gained the casts.
`Data.__setitem__` already restricts the assigned value to the part of it the calling rank owns. Accept any value exposing a `shape` and slicing, such as a memory mapped array, an HDF5 dataset or a file reader, so that it is only read where it is assigned and never read in full by any rank.
The MPI data initialization notebook covers every right-hand side but the one that is not in memory. Show that a value exposing a `shape` and slicing is assigned like a globally replicated array, each rank reading only the window it owns, and note that `initialize_function` inherits the same behaviour.
Updates the requirements on [pip](https://github.com/pypa/pip) to permit the latest version. - [Changelog](https://github.com/pypa/pip/blob/main/NEWS.rst) - [Commits](pypa/pip@9.0.1...26.2) --- updated-dependencies: - dependency-name: pip dependency-version: '26.2' dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Updates the requirements on [distributed](https://github.com/dask/distributed) to permit the latest version. - [Release notes](https://github.com/dask/distributed/releases) - [Changelog](https://github.com/dask/distributed/blob/main/docs/release-procedure.md) - [Commits](dask/distributed@1.1.0...2026.7.1) --- updated-dependencies: - dependency-name: distributed dependency-version: 2026.7.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6 to 7. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6...v7) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Updates the requirements on [fsspec](https://github.com/fsspec/filesystem_spec) to permit the latest version. - [Commits](fsspec/filesystem_spec@0.0.1...2026.7.0) --- updated-dependencies: - dependency-name: fsspec dependency-version: 2026.7.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Updates the requirements on [pytest](https://github.com/pytest-dev/pytest) to permit the latest version. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@7.2.0...9.1.1) --- updated-dependencies: - dependency-name: pytest dependency-version: 9.1.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
The symmetric placement costs an extra inline evaluation of the block, so it is wanted on the handful of equations that need it rather than on a whole operator. `Eq` takes an `interp_mode` and passes it down when it evaluates, leaving `sym_opt` as the default for everything else.
Two changes to what `interp-mode='symmetric'` re-associates. A derivative is where its `x0` puts it, not where the field it differentiates lives. `div(v)` of a staggered velocity lands on the node, and reading the operand's staggering instead made a node-centred kernel look off-node, so it was re-associated onto the operands and its compact stencil replaced by an interpolated one twice as wide. A composite is off only if one of its own operands is, which is what makes a sum of derivatives -- a divergence, a trace -- read as the node-centred quantity it is. The block is then chosen among the factors that are actually off `func`, and it takes two of them for there to be anything to re-associate. With one, the single interpolation `direct` puts on it is already the transpose-consistent form -- it is what makes the `i, j` entry of a stiffness matrix the transpose of its `j, i` entry. With two, `direct` interpolates each separately, and `I(a)*I(b)` is not `I(a*b)`: the discretized operator stops being the transpose of itself, which is invisible in a forward simulation and shows up as a first-order gradient in an adjoint one. The mode also travels down the fallback now. A product that cannot itself be re-associated is routinely wrapped around one that can -- a `dt` scaling, a sum of per-component contractions -- and dropping it there evaluated all of that in `direct`.
A zero-order derivative whose weights collapse to one is the identity, and it comes back as whatever it was applied to. When that is a sum -- the engineering shear strain of a staggered velocity, say -- the rebuild asserted rather than accepting the degenerate result it already handles for a single argument. It is reachable through the symmetric placement of a stiffness contraction.
Four of the five fail without the preceding commits. The fifth guards against over-reach and passes either way: a derivative already sitting on the target must be left alone.
The staggered-interpolation notebook showed the two modes but not a case that needs the symmetric one. Add the gradient case it exists for: a nodal parameter read at a staggered location, and the matching accumulation back onto the node, where the standard placement is off by 4e-1 against 2e-7.
Injects through an Operator built on a grid whose origin differs from the one it is applied to. The linear path regressed here; sinc computes its positions from the runtime `o_x` and did not.
`_arg_defaults` is already handed the runtime object as `self` and the compile-time symbols as `alias`, like the rest of the `_arg_*` protocol, so the frame to tabulate in was there all along. Take the names from `alias` and the data and Grid from `self`, rather than looking the override up in kwargs from `_arg_values`. `_arg_values` now only rebuilds the tables for an explicit `o_x`/`o_y` override, instead of redoing on every call what `_arg_defaults` has already tabulated correctly.
It sits next to `test_position`, which covers the same origin shift through an explicit `o_x`, rather than carrying a class of its own.
Updates the requirements on [packaging](https://github.com/pypa/packaging) to permit the latest version. - [Release notes](https://github.com/pypa/packaging/releases) - [Changelog](https://github.com/pypa/packaging/blob/main/CHANGELOG.rst) - [Commits](pypa/packaging@14.0...26.3) --- updated-dependencies: - dependency-name: packaging dependency-version: '26.3' dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…span
An extraction is scheduled over the Dimensions it spans, but the guard
it inherits from its Cluster is kept as it is. When the guard reads a
Dimension the extraction itself does not span, the temporary is computed
in a loop nest that does not define it, and the generated code does not
compile:
for (int x = ...)
if (sdf(x, y) >= 0) /* y is not iterated here */
r0[y] = ...
which is what an expression guarded by a ConditionalDimension over a
Function of all the Dimensions produces -- an immersed boundary
condition, say -- as soon as a sub-expression of it depends on fewer
Dimensions than the guard. The added test is an MFE of exactly that,
and fails to compile with "'x' undeclared" without this change.
Rule those candidates out in `_do_generate`, next to the extractions
that would break a data dependency, so that they are never generated to
begin with. They cannot go in `exclude` itself, whose entries reject a
candidate that *contains* them, whereas here a candidate is rejected for
what it *lacks*: excluding `x` above would take the whole `x, y`
extraction with it.
A SEQUENTIAL Dimension is exempt: its loop encloses the extraction's
own, so the guard is evaluated outside of them in any case, and
requiring the extraction to span it would cost the hoisting of a time
invariant guarded by a subsampled time Dimension
(`test_invariants_with_conditional`).
The requirement is carried on the transformer rather than passed down
through `_generate`, which is overridden downstream and whose signature
is therefore not ours to change.
The Dimensions a guard reads are what `Guards` is asked for here, so
give it a `dimensions` property rather than walking its values from the
outside. `Cluster.guards_dimensions` was already doing that walk by
hand and now defers to it. The property returns the Dimensions as they
are, not their roots: `guards_dimensions` reaches `expose_tuning_knobs`
through `used_dimensions`, which tests them for `is_Block`, and a root
is never a BlockDimension.
Asking `Properties` whether a Dimension is SEQUENTIAL requires it to
still be one: `CireInvariants._lookup_key` was rebuilding it as a plain
`frozendict`, losing the class and its API, where the other
`_lookup_key` passes it through untouched -- so the two CIRE variants
disagreed on the type. `Properties` is a `frozendict` itself, so
preserving it there costs nothing.
The lock and flag an asynchronous task synchronises on are plain volatile ints,
written by both threads with no ordering imposed. `volatile` guarantees the
loads are re-issued; it does not order stores, so on a weakly ordered target
they can be observed out of order and the handshake loses an update:
compute: lock0[0] = 0; (release_lock0) -- observed late
sdata0->flag = 2; (activate0) -- observed first
task: sees the request, delivers lock0[0] = 2, sets flag = 1
compute: the late lock0[0] = 0 lands, wiping the delivery
the next release_lock0 waits for a 2 nobody will write again
Both threads then spin forever: the compute waiting for data whose request it
has already spent, the task waiting to be asked. `lock == 0 && flag == 1` is
reachable only this way -- the task sets the lock before the flag, so a
completed cycle must leave the lock at 2 -- and that is the state a stalled run
sits in.
Fenced on both sides: a release before the flag that publishes a request or a
completion, an acquire before reading what either stands for. `__atomic_thread_fence`
is a builtin, valid in C and C++ alike, so the device targets that render
through CXXPrinter are unaffected; declaring the two objects `_Atomic` would
have been the standard-clean alternative but that is not valid C++ under g++.
Found through a streaming-checkpoint FWI gradient on arm64, where it stalled one
worker in six within minutes and, in the same runs, had CvxCompress trip its own
assertion on a buffer the task never filled. x86's store ordering hides it.
The tests index the two callables positionally, so they move with the fences.
JDBetteridge
force-pushed
the
JDBetteridge/amd_compression
branch
from
September 9, 2026 12:43
3a87fad to
d51242e
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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
5 participants
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.
Required for PRO PR