compiler: Skip copy-backs overwritten by a later one to the same slot

[mloubout]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.28%. Comparing base (a9964bb) to head (23502c0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
devito/passes/clusters/buffering.py	72.22%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3050      +/-   ##
==========================================
+ Coverage   84.26%   84.28%   +0.01%     
==========================================
  Files         258      258              
  Lines       56982    57053      +71     
  Branches     4854     4869      +15     
==========================================
+ Hits        48018    48087      +69     
+ Misses       8138     8136       -2     
- Partials      826      830       +4     

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	68.52% <35.13%> (-0.05%)	⬇️
pytest-gpu-gcc-	79.04% <86.66%> (+0.01%)	⬆️
pytest-gpu-icx-	78.97% <86.66%> (-0.03%)	⬇️
pytest-gpu-nvc-nvidiaX	69.10% <35.13%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ggorman]

I tested this on CPU and on GPU (RTX 3090, CUDA 12.9), with downstream compressed-snapshot tests and a Devito-only reproducer. Comparisons are against 5c8ec267d (the CSE fix alone).

It fixes the same-slot problem. Downstream tests in which a subsampled save and a CondEq save hit the same slot:

Check	This PR	5c8ec267d
CPU, pinned to one core, 15 runs	15/15 pass	0/15 pass
GPU, async writers, 15 runs	15/15 pass	0/15 pass

When the slot is read between the two writes, both the value read and the final stored value are correct (15/15).

One correctness problem: the later write's condition is evaluated too early. _overwritten places the later copy-back's guards on the earlier copy-back. If those guards read data that is updated between the two writes, they are evaluated on stale data. The earlier copy-back is then skipped, and the later write, seeing the updated data, does not fire, so the slot is never written. Devito-only reproducer (CPU, synchronous):

grid = Grid((4, 4))
time = grid.time_dim
ct0 = ConditionalDimension(name="ct0", parent=time, factor=1, relation=Or)
f = TimeFunction(grid=grid, name='f', time_order=0, space_order=0, time_dim=ct0, save=4)
T = TimeFunction(grid=grid, name='T', time_order=0, space_order=0)
cm = ConditionalDimension(name="cm", parent=time,
                          condition=And(CondEq(time, 3), T[0, 0, 0] < 3.5),
                          relation='strict')
eqs = [Eq(T, time), Eq(f, T), Eq(T, T + 1), Eq(f, T + 1, implicit_dims=cm)]
Operator(eqs, opt='buffering').apply(time_m=0, time_M=3)
# this PR:   f.data[:, 1, 1] == [0, 1, 2, 0]   (slot 3 never written; 3/3 runs)
# 5c8ec267d: f.data[:, 1, 1] == [0, 1, 2, 3]   (correct)

Control: drop only Eq(T, T + 1). Both revisions are then correct, giving [0, 1, 2, 4], and this PR still skips the first copy-back.

Suggested fix: don't skip when the later guards read anything written between the two Clusters. This needs retrieve_terminals imported in buffering.py and clusters passed to _overwritten:

-            conditions.append(And(*c1.guards.values(), coincide))
+            # The later guards are evaluated at the earlier copy-back, so they
+            # must not read anything written in between
+            condition = And(*c1.guards.values(), coincide)
+            deps = {i.function for i in retrieve_terminals(condition)}
+            if any(deps & set(c2.scope.writes)
+                   for c2 in clusters[order[c]:order[c1]]):
+                continue
+
+            conditions.append(condition)

With this change:

• The reproducer gives [0, 1, 2, 3].
• The variant without the update still gets the skip.
• tests/test_buffering.py passes (60 passed, 1 xfailed).
• The downstream same-slot tests still pass, including one for this case. (CPU only; I haven't rerun the GPU tests with it.)

It would be worth adding the reproducer as a test.

Smaller points

• The v1.b is v.b exclusion keeps both copy-backs when they come from the same buffer. That is fine for a memcpy, but for an append-only store (e.g. compressed snapshots) two serialized copy-backs are still two appends: the first leaves an orphaned chunk, and at exact capacity a write is silently dropped. I haven't checked whether user-level equations can produce a same-buffer case. If they can, it's worth handling or documenting.
• This PR also contains 5c8ec267d, which is #3047. Either rebase once #3047 is merged, or close #3047 in favour of this PR.
• The description is empty. A short statement of the problem would help: two copy-backs to one slot in the same iteration are redundant, and racy with asynchronous copy-backs. So would the guard semantics.

[mloubout]

This new MFE doesn't make sense it doesn't have a correct answer. It has two write with different values at the same time step it cannot have a correct answer and the solution proposed is just an arbitrary choice.

[ggorman]

Follow-up: two more CPU cases where this gives wrong results. Both are correct on 5c8ec267d.

1. A read under a different guard, between the two writes, sees an unwritten slot. The guard-dependency fix I suggested above does not cover this.

grid = Grid((4, 4))
time = grid.time_dim
ct0 = ConditionalDimension(name='ct0', parent=time, factor=1, relation=Or)
f = TimeFunction(grid=grid, name='f', time_order=0, space_order=0, time_dim=ct0, save=4)
g = TimeFunction(grid=grid, name='g', time_order=0, space_order=0, save=4)
T = TimeFunction(grid=grid, name='T', time_order=0, space_order=0)
cr = ConditionalDimension(name='cr', parent=time, condition=time >= 2, relation='strict')
cm = ConditionalDimension(name='cm', parent=time, condition=CondEq(time, 3), relation='strict')
eqs = [Eq(T, time), Eq(f, T), Eq(g, f, implicit_dims=cr), Eq(f, T + 1, implicit_dims=cm)]
Operator(eqs, opt='buffering').apply(time_m=0, time_M=3)
# this PR:   g.data[:, 1, 1] == [0, 0, 2, 0]
# 5c8ec267d: g.data[:, 1, 1] == [0, 0, 2, 3]   (correct)

The earlier copy-back is skipped, but g reads f before the later write lands. The final f is correct, so a test that checks only f misses this. _overwritten doesn't check for reads in between.

2. The guard-dependency fix misses indirect reads. If the later guard is T[0, I[0, 0, 0], 0] < 3.5 and only I changes between the writes, the result is still wrong ([2, 3, 4, 0] instead of [2, 3, 4, 5]). retrieve_terminals(..., deep=True) catches this one.

Could these, and the mutable-guard case above, be covered before merge? Happy to share the full scripts.

[mloubout]

This is still the same "mfe" with no correct answer you can't do Eq(f, val1) and Eq(f, val2) at the same time step this doesnt have a valid correct answer

[JDBetteridge]

I'm merging this as we need it to fix things broken upstream, feel free to open an issue or a new PR to address any outstanding nuances