Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Patch race conditions due to storage-related dependencies #1903

Merged
merged 5 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions devito/ir/support/basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ def aindices(self):
retval.append(dims.pop())
elif isinstance(i, Dimension):
retval.append(i)
elif q_constant(i):
retval.append(fi)
else:
retval.append(None)
return DimensionTuple(*retval, getters=self.findices)
Expand Down Expand Up @@ -262,10 +260,14 @@ def is_regular(self):
# space Dimensions
positions = []
for d in self.aindices:
for n, i in enumerate(self.intervals):
if i.dim._defines & d._defines:
positions.append(n)
break
try:
for n, i in enumerate(self.intervals):
if i.dim._defines & d._defines:
positions.append(n)
break
except AttributeError:
# `d is None` due to e.g. constant access
continue
return positions == sorted(positions)

def __lt__(self, other):
Expand Down Expand Up @@ -548,6 +550,15 @@ def is_cross(self):
def is_local(self):
return self.function.is_Symbol

@memoized_meth
def is_const(self, dim):
"""
True if a constant depedence, that is no Dimensions involved, False otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo depedence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"""
return (self.source.aindices[dim] is None and
self.sink.aindices[dim] is None and
self.distance_mapper[dim] == 0)

@memoized_meth
def is_carried(self, dim=None):
"""Return True if definitely a dimension-carried dependence, False otherwise."""
Expand Down Expand Up @@ -623,9 +634,10 @@ def is_storage_related(self, dims=None):
cause the access of the same memory location, False otherwise.
"""
for d in self.findices:
if (d._defines & set(as_tuple(dims)) and
any(i.is_NonlinearDerived for i in d._defines)):
return True
if d._defines & set(as_tuple(dims)):
if any(i.is_NonlinearDerived for i in d._defines) or \
self.is_const(d):
return True
return False


Expand Down
9 changes: 7 additions & 2 deletions devito/mpi/halo_scheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ def classify(exprs, ispace):
v[(d, LEFT)] = STENCIL
v[(d, RIGHT)] = STENCIL
else:
v[(d, i.aindices[d])] = NONE
v[(d, i[d])] = NONE

# Does `i` actually require a halo exchange?
if not any(hl is STENCIL for hl in v.values()):
Expand Down Expand Up @@ -426,7 +426,12 @@ def classify(exprs, ispace):
func = Max
candidates = [i for i in aindices if not is_integer(i)]
candidates = {(i.origin if d.is_Stepping else i) - d: i for i in candidates}
loc_indices[d] = candidates[func(*candidates.keys())]
try:
loc_indices[d] = candidates[func(*candidates.keys())]
except KeyError:
# E.g., `aindices = [0, 1, d+1]` -- it doesn't really matter
# what we put here, so we place 0 as it's the old behaviour
loc_indices[d] = 0

mapper[f] = HaloSchemeEntry(frozendict(loc_indices), frozenset(halos))

Expand Down
2 changes: 1 addition & 1 deletion devito/passes/clusters/asynchrony.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def callback(self, clusters, prefix):
else:
# Functions over non-stepping Dimensions need no lock
continue
except KeyError:
except (AttributeError, KeyError):
# Would degenerate to a scalar, but we rather use a lock
# of size 1 for simplicity
ld = CustomDimension(name='ld', symbolic_size=1)
Expand Down
25 changes: 22 additions & 3 deletions devito/passes/clusters/buffering.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
import numpy as np

from devito.ir import (Cluster, Forward, GuardBound, Interval, IntervalGroup,
IterationSpace, PARALLEL, Queue, Vector, lower_exprs, vmax, vmin)
IterationSpace, PARALLEL, Queue, SEQUENTIAL, Vector,
lower_exprs, normalize_properties, vmax, vmin)
from devito.exceptions import InvalidOperator
from devito.logger import warning
from devito.symbolics import retrieve_function_carriers, uxreplace
Expand Down Expand Up @@ -207,7 +208,16 @@ def callback(self, clusters, prefix, cache=None):
expr = lower_exprs(uxreplace(Eq(lhs, rhs), b.subdims_mapper))
ispace = b.written

processed.append(c.rebuild(exprs=expr, ispace=ispace))
# Buffering creates a storage-related dependence along the
# contracted dimensions
properties = dict(c.properties)
for d in b.contraction_mapper:
d = ispace[d].dim # E.g., `time_sub -> time`
properties[d] = normalize_properties(properties[d], {SEQUENTIAL})

processed.append(
c.rebuild(exprs=expr, ispace=ispace, properties=properties)
)

# Substitute buffered Functions with the newly created buffers
exprs = [uxreplace(e, subs) for e in c.exprs]
Expand All @@ -233,7 +243,16 @@ def callback(self, clusters, prefix, cache=None):
expr = lower_exprs(uxreplace(Eq(lhs, rhs), b.subdims_mapper))
ispace = b.written

processed.append(c.rebuild(exprs=expr, ispace=ispace))
# Buffering creates a storage-related dependence along the
# contracted dimensions
properties = dict(c.properties)
mloubout marked this conversation as resolved.
Show resolved Hide resolved
for d in b.contraction_mapper:
d = ispace[d].dim # E.g., `time_sub -> time`
properties[d] = normalize_properties(properties[d], {SEQUENTIAL})

processed.append(
c.rebuild(exprs=expr, ispace=ispace, properties=properties)
)

return processed

Expand Down
19 changes: 19 additions & 0 deletions tests/test_buffering.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,25 @@ def test_multi_access():
assert np.all(w.data == w1.data)


def test_issue_1901():
grid = Grid(shape=(2, 2))
time = grid.time_dim
x, y = grid.dimensions

usave = TimeFunction(name='usave', grid=grid, save=10)
v = TimeFunction(name='v', grid=grid)

eq = [Eq(v[time, x, y], usave)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: Eq(usave, v) bit more readable and makes bit more "sense"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want time in place of t since otherwise the loop will be tagged SEQUENTIAL due to the presence of uindices. But I want it fully PARALLEL to trigger the issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fin,e I meant having usave as lhs mostly but again just nitpicking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see what you mean now...
well in backward propagators you do have usave's on the RHS :)


op = Operator(eq, opt='buffering')

trees = retrieve_iteration_tree(op)
assert len(trees) == 2
assert trees[1].root.dim is time
assert not trees[1].root.is_Parallel
assert trees[1].root.is_Sequential # Obv


def test_everything():
nt = 50
grid = Grid(shape=(6, 6))
Expand Down
2 changes: 1 addition & 1 deletion tests/test_dle.py
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ def test_incs_no_atomic(self):
@pytest.mark.parametrize('exprs,simd_level,expected', [
(['Eq(y.symbolic_max, g[0, x], implicit_dims=(t, x))',
'Inc(h1[0, 0], 1, implicit_dims=(t, x, y))'],
2, [6, 0, 0]),
None, [6, 0, 0]),
(['Eq(y.symbolic_max, g[0, x], implicit_dims=(t, x))',
'Eq(h1[0, y], y, implicit_dims=(t, x, y))'], # 1695
2, [0, 1, 2]),
Expand Down
8 changes: 6 additions & 2 deletions tests/test_ir.py
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ def test_multiple_eqs(self, exprs, expected, ti0, ti1, ti3, fa):
assert len(expected) == 0


class TestAnalysis(object):
class TestParallelismAnalysis(object):

@pytest.mark.parametrize('exprs,atomic,parallel', [
(['Inc(u[gp[p, 0]+rx, gp[p, 1]+ry], cx*cy*src)'],
Expand All @@ -726,7 +726,10 @@ class TestAnalysis(object):
(['Eq(v.forward, v[t+1, x, y-1]+v[t, x, y]+v[t, x, y-1])'],
[], ['x']),
(['Eq(v.forward, v+1)', 'Inc(u, v)'],
[], ['x', 'y'])
[], ['x', 'y']),
# Test for issue #1902
(['Eq(u[0, y], v)', 'Eq(w, u[0, y])'],
[], ['y'])
])
def test_iteration_parallelism_2d(self, exprs, atomic, parallel):
"""Tests detection of PARALLEL_* properties."""
Expand All @@ -742,6 +745,7 @@ def test_iteration_parallelism_2d(self, exprs, atomic, parallel):

u = Function(name='u', grid=grid) # noqa
v = TimeFunction(name='v', grid=grid, save=None) # noqa
w = TimeFunction(name='w', grid=grid, save=None) # noqa

cx = Function(name='coeff_x', dimensions=(p, rx), shape=(1, 2)) # noqa
cy = Function(name='coeff_y', dimensions=(p, ry), shape=(1, 2)) # noqa
Expand Down