compiler: Fix two guard bugs exposed by buffering by ggorman · Pull Request #3061 · devitocodes/devito

ggorman · 2026-10-16T23:02:01Z

Two guard bugs that buffering exposes, each covered by a regression test:

if (false) was emitted. A condition SymPy folds to false (e.g. time < 0) printed if (false), which is not C99 without stdbool.h: a compile error. True and false now print as 1 and 0 in C output (the C++ printer already emits true/false, which C++ accepts).
{time: true} guards. time >= 0 folds to true and gave a Cluster a guard distinct from no guard, so buffering created one buffer per "guard" for equivalent accesses. Guards now never sets a guard that reduces to true, whatever the entry point.

Tests: test_dimension.py::TestConditionalDimension::test_false_condition, test_buffering.py::test_true_condition_single_buffer and test_dimension.py::TestConditionalDimension::test_true_never_set. Each fails without its source change.

An earlier GuardFactorNe change was dropped ("compiler: Drop the GuardFactorNe change and its test"): no real program reaches a negated factor guard.

First of three stacked PRs: this one, then #3062 (copy-back skip rule) and #3063 (stale reads and lost writes).

🤖 Generated with Claude Code

codecov · 2026-10-16T23:16:47Z

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.27%. Comparing base (a9e3f31) to head (953054a).

Files with missing lines	Patch %	Lines
devito/passes/iet/languages/C.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3061      +/-   ##
==========================================
- Coverage   84.27%   84.27%   -0.01%     
==========================================
  Files         258      258              
  Lines       57421    57473      +52     
  Branches     4911     4913       +2     
==========================================
+ Hits        48392    48434      +42     
- Misses       8205     8214       +9     
- Partials      824      825       +1

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`68.49% <75.00%> (+<0.01%)`	⬆️
pytest-gpu-gcc-	`78.91% <98.11%> (+0.01%)`	⬆️
pytest-gpu-icx-	`78.83% <98.11%> (+0.02%)`	⬆️
pytest-gpu-nvc-nvidiaX	`69.21% <75.00%> (+<0.01%)`	⬆️

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.

mloubout

seems sane

FabioLuporini · 2026-10-19T08:43:41Z

                guards = {d: mode(*v, evaluate=False) for d, v in guards.items()}

+            # A condition folded to `true` (e.g., `time >= 0`) imposes no guard
+            guards = {d: v for d, v in guards.items() if v is not sympy.true}


I think this should rather be fixed directly in class Guards, within devito/ir/support/guards.py: a guard, along any Dimension, should never be set (being pointless) if its value reduces to sympy.true

Done in 4c8c1cd: Guards.__init__ now drops any entry equal to sympy.true, so no entry point (constructor, andg, xandg, impose, subs, ...) can set one, and the filter in the guard pass is gone. Added TestGuards::test_true_never_set; the buffering test still covers the original case.

mloubout · 2026-10-19T11:53:04Z

+
+        ct = ConditionalDimension(name='ct', parent=time, factor=2)
+        cn = ConditionalDimension(name='cn', parent=time,
+                                  condition=Not(GuardFactor(ct)), relation='strict')


GuardFactor should not be used at this level this is an internal object, this needs to be a user API def, like Not(CondEq(...

Fair point, fixed in 763b8b2. GuardFactor is only ever built by the compiler from a ConditionalDimension(factor=...), and its negation only arises in buffering, when a copy-back is overwritten by a later one guarded by the factor dimension — so the test now goes through that: two writes to the same usave slot, one at the last timestep and one under factor=2, noop vs buffering (test_buffering.py::test_negated_factor_guard). Fails on main with if (time == 7 && time%tsubf == 0), passes with != 0. The old test is gone from test_dimension.py.

mloubout · 2026-10-19T12:35:29Z

+    assert np.allclose(h.data[0], 12.)
+
+
+def test_negated_factor_guard():


that's an abuse of the API,

the usave isn't defined on tsub but on time

It saves two different values at the same steps which doesn't make sense.

You're right, it was contrived. I checked it against real code (recipes, devitohorizon, the devito examples and the devitopro demos/tests): every snapshot schedule saves on the subsampled dimension with a single writer, and the only same-slot two-writer case is the last-step save in your test_multi_cond_same_slot, which negates the CondEq guard, not the factor one, and is unaffected by the GuardFactorNe change. So nothing real reaches a negated factor guard, and I've dropped both that change and the test (4d820ac). The PR is now just the true/false guard fixes.

FabioLuporini · 2026-10-20T08:58:27Z

+        super().__init__(*args, **kwargs)
+
+        # A guard that reduces to `true` is pointless, hence never set
+        self._dict = {d: v for d, v in self._dict.items() if v != true}


what's the point of a _dict inside __init__? this is nonsensical

oh _dict is the internal of frozendict, I guess...

well then u should rather apply the filtering to args/kwargs, not change _dict subsequently after calling into super()

Right, that was poking frozendict's internals. Now Guards.__init__ builds the mapper from args/kwargs, drops the true entries and then calls super().__init__(mapper), as Properties does.

FabioLuporini · 2026-10-22T08:44:52Z

 class TestGuards:

+    @pytest.mark.parametrize('relation', [And, Or, 'strict'])
+    def test_true_never_set(self, relation):


this test belongs to test_dimensions.py::TestConditionalDimension (and it should be placed at the bottom of the class, not at the top)

Moved to the bottom of TestConditionalDimension in 953054a; test_ir.py is back to what main has.

GuardFactorNe inherited CondEq through GuardFactor ahead of CondNe, so it was an equality and printed `==`. Negating a subsampling guard thus reproduced the guard itself, selecting the timesteps on the subsampled grid instead of those off it

A condition SymPy folds to `false`, e.g. `time < 0`, was printed as `if (false)`, which is not C99 without `stdbool.h` and failed to compile

A condition such as `time >= 0` folds to `true` (`time` is nonnegative) and gave a Cluster the guard {time: true}, distinct from no guard. Buffering then created one buffer per guard for equivalent accesses

Drop such guards in `Guards` itself, at construction, rather than in the `guard` pass, so that no entry point (`andg`, `xandg`, `impose`, `subs`, ...) can set one

The negated subsampling guard is reached through buffering, when a copy-back is overwritten by a later one under a factor-based ConditionalDimension, so test it there instead of constructing the internal GuardFactor

A ConditionalDimension whose condition folds to true must yield no Conditional in the generated code, under any relation

No user program reaches a negated factor guard: real snapshot schedules save on the subsampled dimension with a single writer, and the same-slot last-step save negates a CondEq instead. The test needed a contrived program to get there, so drop both

ggorman · 2026-10-22T12:47:26Z

The osx failure is the Apple-silicon detection flake, not this change: the job header reads platform: cpu64 on the arm64 runner, so the cpu64-arm skips do not apply. test_nested_nthreads then fails compiling x86 intrinsic headers and test_gradient_equivalence[...float64-1e-13...] runs instead of being skipped (https://github.com/devitocodes/devito/actions/runs/39770949975/job/119798044208). #3048 fixes the detection. The other 41 jobs pass.

ggorman added the compiler label Oct 16, 2026

This was referenced Oct 16, 2026

compiler: Skip copy-backs only if no write to the guard intervenes #3062

Draft

compiler: Fix stale reads and lost writes in buffering #3063

Draft

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

Merged

mloubout reviewed Oct 19, 2026

View reviewed changes

FabioLuporini requested changes Oct 19, 2026

View reviewed changes

mloubout reviewed Oct 19, 2026

View reviewed changes

mloubout approved these changes Oct 19, 2026

View reviewed changes

ggorman changed the title ~~compiler: Fix three guard bugs exposed by buffering~~ compiler: Fix two guard bugs exposed by buffering Oct 19, 2026

ggorman marked this pull request as ready for review October 19, 2026 13:32

ggorman requested a review from FabioLuporini October 19, 2026 13:32

FabioLuporini requested changes Oct 20, 2026

View reviewed changes

ggorman force-pushed the fix-buffering-guards branch from 4d820ac to cdd0ce8 Compare October 20, 2026 09:48

ggorman marked this pull request as draft October 20, 2026 10:14

ggorman marked this pull request as ready for review October 21, 2026 22:34

ggorman requested review from FabioLuporini and mloubout October 21, 2026 22:34

FabioLuporini requested changes Oct 22, 2026

View reviewed changes

ggorman and others added 8 commits October 22, 2026 11:14

compiler: Make GuardFactorNe an inequality

9f61204

GuardFactorNe inherited CondEq through GuardFactor ahead of CondNe, so it was an equality and printed `==`. Negating a subsampling guard thus reproduced the guard itself, selecting the timesteps on the subsampled grid instead of those off it

compiler: Print SymPy true/false as 1/0 in C

1a8fc6d

A condition SymPy folds to `false`, e.g. `time < 0`, was printed as `if (false)`, which is not C99 without `stdbool.h` and failed to compile

compiler: Drop cluster guards folded to true

d46c0bd

A condition such as `time >= 0` folds to `true` (`time` is nonnegative) and gave a Cluster the guard {time: true}, distinct from no guard. Buffering then created one buffer per guard for equivalent accesses

compiler: Never set a guard reducing to true

36cda5b

Drop such guards in `Guards` itself, at construction, rather than in the `guard` pass, so that no entry point (`andg`, `xandg`, `impose`, `subs`, ...) can set one

tests: Exercise GuardFactorNe through the user API

242a160

The negated subsampling guard is reached through buffering, when a copy-back is overwritten by a later one under a factor-based ConditionalDimension, so test it there instead of constructing the internal GuardFactor

tests: Check a true guard is never set through the user API

b27da4c

A ConditionalDimension whose condition folds to true must yield no Conditional in the generated code, under any relation

tests: Move the true-guard test to TestConditionalDimension

953054a

ggorman force-pushed the fix-buffering-guards branch from cdd0ce8 to 953054a Compare October 22, 2026 11:29

ggorman requested a review from FabioLuporini October 22, 2026 11:29

		assert np.allclose(h.data[0], 12.)


		def test_negated_factor_guard():

Conversation

ggorman commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mloubout left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggorman commented Oct 22, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggorman commented Oct 16, 2026 •

edited

Loading

codecov Bot commented Oct 16, 2026 •

edited

Loading