api: fix handling of multiple conditions for buffering by mloubout · Pull Request #2850 · devitocodes/devito

mloubout · 2026-02-16T18:55:59Z

No description provided.

codecov · 2026-02-16T19:13:17Z

Codecov Report

❌ Patch coverage is 85.04673% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.66%. Comparing base (3df12db) to head (0eb1800).

Files with missing lines	Patch %	Lines
devito/passes/clusters/buffering.py	80.29%	17 Missing and 10 partials ⚠️
devito/passes/clusters/asynchrony.py	75.00%	11 Missing and 3 partials ⚠️
devito/ir/support/guards.py	75.92%	11 Missing and 2 partials ⚠️
devito/ir/stree/algorithms.py	20.00%	3 Missing and 1 partial ⚠️
devito/types/utils.py	20.00%	4 Missing ⚠️
devito/ir/clusters/algorithms.py	81.81%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2850      +/-   ##
==========================================
+ Coverage   83.64%   83.66%   +0.01%     
==========================================
  Files         257      257              
  Lines       54235    54524     +289     
  Branches     4629     4680      +51     
==========================================
+ Hits        45367    45618     +251     
- Misses       8072     8099      +27     
- Partials      796      807      +11

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`68.52% <68.66%> (-0.02%)`	⬇️
pytest-gpu-gcc-	`78.27% <74.29%> (-0.03%)`	⬇️
pytest-gpu-icx-	`78.21% <74.29%> (-0.02%)`	⬇️
pytest-gpu-nvc-nvidiaX	`69.18% <68.66%> (-0.02%)`	⬇️

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.

FabioLuporini · 2026-02-20T08:56:53Z

        return CondNe(*self.args, evaluate=False)

+    @property
+    def _as_min(self):


I would drop this and rather have a singledispatch handler for CondEq where necessary

FabioLuporini · 2026-02-20T08:58:34Z

-            expr = uxreplace(expr, {d: IntDiv(index, d.symbolic_factor)})
+
+        # Merge conditionals when possible. E.g if we have an implicit_dim
+        # and there is a dimension with the same parent, we ca merged


Dimension

"ca merged"

"their conditions"

you could also make the example a bit more practical

FabioLuporini · 2026-02-20T08:59:22Z

+        for d in input_expr.implicit_dims:
+            if d not in conditionals:
+                continue
+            for cd in dict(conditionals):


list(...) is fine

FabioLuporini · 2026-02-20T09:05:57Z

+        # Replace the ConditionalDimensions in `expr`
+        for d, cond in conditionals.items():
+            # Replace dimension with index
+            index = d.index


you can spare this line

FabioLuporini · 2026-02-20T09:07:18Z

        ispace = IterationSpace(intervals, iterators)

-        # Construct the conditionals and replace the ConditionalDimensions in `expr`
+        # Construct the conditionals


I think we should place this whole block of code, which constructs/lowers the conditionals, into its own separate functions, and a docstring with some examples

review-notebook-app · 2026-05-28T17:17:37Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini · 2026-05-30T13:11:27Z

+            shift = relational_shift(cond, d.parent)
+            expr = uxreplace(expr, {d: IntDiv(index, d.symbolic_factor) + shift})
+
+        # Merge conditionals when possible. E.g if we have an implicit_dim


btw this block imho deserves its own function

FabioLuporini · 2026-05-30T13:12:30Z

            if d is not dim:
                continue
-
+            if d in c0.guards and not c0.guards[d].has(Mod):


searching for Mod is a bit meh, I'd rather add a special guard to ir/support/guards.py and look for that instead (there's quite a few already in there!)

FabioLuporini · 2026-05-30T13:12:51Z

            _actions_from_update_memcpy(c, d, clusters, actions, sregistry)
        elif d.is_Custom and is_integer(c.ispace[d].size):
-            _actions_from_init(c, d, actions)
+            _actions_from_init(c, d, clusters, actions)


leftover, I guess

FabioLuporini · 2026-05-30T13:12:55Z



-def _actions_from_init(c, d, actions):
+def _actions_from_init(c, d, clusters, actions):


leftover, I guess

FabioLuporini · 2026-07-13T07:55:33Z

+        if d not in conditionals:
+            continue
+        for cd in list(conditionals):
+            if cd.parent == d.parent and cd is not d:


nitpicking: cd.parent is d.parent ...

FabioLuporini · 2026-07-13T07:56:42Z

+            continue
+        for cd in list(conditionals):
+            if cd.parent == d.parent and cd is not d:
+                cond = conditionals.pop(d)


nitpicking: you don't need this line here, you may just .pop in the else branch, and avoid the = cond in the if branch

FabioLuporini · 2026-07-13T07:56:57Z

+
+    # Replace the ConditionalDimensions in `expr`
+    for d, cond in conditionals.items():
+        # Replace dimension with index


redundant comment?

FabioLuporini · 2026-07-13T09:24:38Z

+            g = c0.guards.get(d)
+            # Explicit compute guards need no pipeline; memcpy clusters
+            # still need WithLock for the copy-back sync
+            if g is not None and not wraps_memcpy(c0):


I think a lot of the complexity inherent in this guard and the nested one could be mitigated by simply introducing somehting along the lines of c0.guards.has(d, GuardFactor)

FabioLuporini · 2026-07-13T09:46:44Z

+
+    # Alias deferred entries to an existing f-buffer; create one if none
+    for f, deferred in extras.items():
+        reusable = [mapper[(ff, kk)] for (ff, kk) in mapper if ff == f]


FabioLuporini · 2026-07-13T09:49:24Z

+                    # Add all missing buffering dimensions and reorder to
+                    # avoid duplicates with different ordering
+                    ispaces.add(c.ispace.insert(self.dim, edims).reorder())
+            elif ((self.f in c.scope.reads or self.f in c.scope.writes) and


self.f in c.scope.functions or in fact self.f in c.functions should suffice

FabioLuporini · 2026-07-13T09:50:15Z

-
-            lhs = b.indexify()._subs(v.xd, v.first_idx.b)
-            rhs = f.indexify()._subs(v.dim, v.first_idx.f)
+    for b, vb in descriptors.items():


another potential occurrence of descriptors.flatten

FabioLuporini · 2026-07-13T09:52:17Z

-                    )
-                else:
-                    size = async_degree
+        for k, ck in groupby(clusters, key=lambda c: c.guards):


ultra-uber-nitpicking, typically we use letter g for groups

FabioLuporini · 2026-07-13T09:54:10Z

    return p, offset
+
+
+def _buffer_dim(f, key, ck):


this is potentially a legacy over-complication

is it basically what in PRO refer to as .hdim ?

because imho, if so, we could/should somehow abstract the concept here, and just use f.hdim (or whatever name you wanna give it)

FabioLuporini · 2026-08-14T08:36:34Z

            # the purpose of protecting from OOB accesses
            cds = [d for d in cds if not d.indirect]
+            modes = [cd.relation for cd in cds]
+            strict = ConditionalDimension._STRICT


STRICT instead of _STRICT since it's not a private class attribute

FabioLuporini · 2026-08-14T08:37:32Z

+            # still need WithLock for the copy-back sync. An "explicit"
+            # guard is a plain relation on `d` (e.g., `d == K`); subsampling
+            # guards (GuardFactor) still require the standard async pipeline
+            if d in c0.guards and not wraps_memcpy(c0) \


ultra nitpick, for homogeneity we typically do:

if d in c0.guards and \ not wraps_memcpy(c0) and \ not c0.guards.has(d, GuardFactor):

blank lines surrounding this new block would help also

FabioLuporini · 2026-08-14T08:39:01Z

-        descriptors = {b: BufferDescriptor(f, b, bfmap[f])
-                       for f, b in self.mapper.items()
-                       if f in bfmap}
+        descriptors = {}


ultra-nitpick memo

FabioLuporini · 2026-08-14T08:40:04Z

                properties = c.properties.sequentialize(d)
-                if not isinstance(d, BufferDimension):
+                if not isinstance(d, BufferDimension) and \
+                   not _explicit_guard(c.guards.get(d, None), d):


since _explicit_guard is called only here, you can probably move the not isinstance(d, BufferDimension) part of the condition inside of it, thus simplifying this caller site

FabioLuporini · 2026-08-14T08:40:41Z

-            index_mapper = {i: mds[(v.xd, i)] for i in v.indices}
-            for a in accesses:
-                subs[a.access] = b.indexed[[index_mapper.get(i, i) for i in a]]
+        subs = defaultdict(dict)


I'd do this, it'd shorten this loop body significantly

FabioLuporini · 2026-08-14T08:46:20Z

+
+    cls = callback or Array
+    name = sregistry.make_name(prefix=f'{f.name}b')
+    return cls(name=name, dimensions=dimensions, dtype=f.dtype,


u now might be able to fit this in two lines

FabioLuporini · 2026-08-14T08:51:24Z

+
+        # Indirect accessors define the ispace only for a read-only streamed
+        # buffer, where nothing iterates the buffer's own Dimensions directly
+        if not ispaces:


this could be glued with the if len(ispaces) > 1:, u could have:

if len(ispaces) == 0: # Indirect accessors define the ispace only for a read-only streamed # buffer, where nothing iterates the buffer's own Dimensions directly ispace = indirect.pop() elif len(ispaces) == 1: # <comment> ispace = ispaces.pop() else: # Best effort to make buffering work in the presence of multiple # IterationSpaces <code> ispace = ispaces.pop() return ispace

in retrospect, do you really need indirect ? are u sure u cannot just reuse the same ispaces ? I guess u know better than me...

actually can probably just use ispaces yeah, droping indirect

FabioLuporini · 2026-08-14T08:55:45Z

+            buf = next((b for b in reusable
+                        if set(k) & set(b.dimensions)), None)
+            if buf is None and reusable:
+                buf = reusable[0]


are u expecting len(reusable) == 1 ? if yes, is it worth asserting it? if not, why do we pick [0]

No, that was lazy, tweked for better selection

FabioLuporini · 2026-08-14T08:58:26Z

+
+
+def _make_buffer(f, dim, k, ck, xds, async_degree, sregistry, callback):
+    """Build (or retrieve) the buffer Array for `f` along `dim` under guards `k`."""


then instead of _make_buffer I'd call it differently (pick_buffer ?) and remove the parentheses surrounding "or retrieve" since it's very important , not a minor detail

FabioLuporini · 2026-08-14T08:59:47Z

                return self.getters[d]
        raise KeyError

+    def dindex(self, dim):


why not just a def index override? if the arg is an integer you call into super(), otherwise do the logic below

It's only used in one place (for transforms) and it makes the standard index work better as the dict's default one

mloubout added the API api (symbolics, types, ...) label Feb 16, 2026

FabioLuporini reviewed Feb 20, 2026

View reviewed changes

mloubout force-pushed the multi-cond-again branch 4 times, most recently from ef708e5 to b997156 Compare May 22, 2026 15:13

mloubout force-pushed the multi-cond-again branch 5 times, most recently from 7a1a6aa to c7786ea Compare May 28, 2026 17:17

mloubout force-pushed the multi-cond-again branch 11 times, most recently from f904760 to 0500469 Compare May 29, 2026 16:45

FabioLuporini reviewed May 30, 2026

View reviewed changes

Comment thread examples/userapi/05_conditional_dimension.ipynb

mloubout force-pushed the multi-cond-again branch from 0500469 to 30790f0 Compare May 30, 2026 14:35

mloubout force-pushed the multi-cond-again branch from 89aea51 to 99f0a9e Compare July 3, 2026 07:41

FabioLuporini reviewed Jul 13, 2026

View reviewed changes

mloubout mentioned this pull request Jul 20, 2026

compiler: Revamp fuse-task optoption #2975

Merged

mloubout force-pushed the multi-cond-again branch from 99f0a9e to bebbf4f Compare July 21, 2026 09:42

mloubout force-pushed the multi-cond-again branch 6 times, most recently from ab8cd52 to b1147aa Compare July 30, 2026 22:25

FabioLuporini reviewed Aug 5, 2026

View reviewed changes

Comment thread devito/ir/equations/algorithms.py

FabioLuporini reviewed Aug 5, 2026

View reviewed changes

Comment thread devito/ir/stree/algorithms.py

mloubout force-pushed the multi-cond-again branch 6 times, most recently from 267a4ed to cfa4221 Compare August 13, 2026 18:34

FabioLuporini reviewed Aug 14, 2026

View reviewed changes

mloubout added 10 commits August 14, 2026 09:01

compiler: fix multi-cond async

d696748

compiler: fix multi-cond for multi-layer

e6fd96b

api: fix interpolate with complex dtype

d6565a5

api: fix buffering with multiple conditions

0aeef75

compiler: fix various corner case of multi buffering

60f7897

compiler: support mutli-buffering

0771802

misc: cleanup and add comments

05aa7af

compiler: fix buffering corner case such as interp

09ae281

compiler: add hdim for buffering and better guard logic

f92847e

compiler: last cleanup pass through buffering

0eb1800

FabioLuporini approved these changes Aug 14, 2026

View reviewed changes



		def _actions_from_init(c, d, actions):
		def _actions_from_init(c, d, clusters, actions):



		def _make_buffer(f, dim, k, ck, xds, async_degree, sregistry, callback):
		"""Build (or retrieve) the buffer Array for `f` along `dim` under guards `k`."""

Uh oh!

Conversation

mloubout commented Feb 16, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

review-notebook-app Bot commented May 28, 2026

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!

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!

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!

Uh oh!

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!

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

codecov Bot commented Feb 16, 2026 •

edited

Loading