From 6df57f68a577fa6b404b7686ad0af019900850be Mon Sep 17 00:00:00 2001 From: Matt Wala Date: Mon, 8 Apr 2019 15:44:29 -0500 Subject: [PATCH 1/3] Improve loopy codegen determinism Use an ordered set for collecting operators to ensure deterministic variable naming, as the variable name depends on the collection order. (As an alternative I tried sorting the output of the collector by string, but the stringification is very expensive on simple examples like wave-min.) Also, improve debuggability of any remaining bugs in loopy kernels (such as any latent nondeterminism) by giving each assignment kernel a separate name. --- grudge/symbolic/compiler.py | 4 ++ grudge/symbolic/mappers/__init__.py | 15 ++++-- grudge/tools.py | 73 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 5 deletions(-) diff --git a/grudge/symbolic/compiler.py b/grudge/symbolic/compiler.py index b27d5585..f8dc173e 100644 --- a/grudge/symbolic/compiler.py +++ b/grudge/symbolic/compiler.py @@ -968,6 +968,7 @@ def bessel_function_mangler(kernel, name, arg_dtypes): class ToLoopyInstructionMapper(object): def __init__(self, dd_inference_mapper): self.dd_inference_mapper = dd_inference_mapper + self.insn_count = 0 def map_insn_assign(self, insn): from grudge.symbolic.primitives import OperatorBinding @@ -1007,6 +1008,7 @@ class ToLoopyInstructionMapper(object): insns, default_offset=lp.auto, + name="grudge_assign_%d" % self.insn_count, # Single-insn kernels may have their no_sync_with resolve to an # empty set, that's OK. options=lp.Options(check_dep_resolution=False)) @@ -1014,6 +1016,8 @@ class ToLoopyInstructionMapper(object): knl = lp.set_options(knl, return_dict=True) knl = lp.split_iname(knl, iname, 128, outer_tag="g.0", inner_tag="l.0") + self.insn_count += 1 + from pytools import single_valued governing_dd = single_valued( self.dd_inference_mapper(expr) diff --git a/grudge/symbolic/mappers/__init__.py b/grudge/symbolic/mappers/__init__.py index d52f7ac5..e47bd6c2 100644 --- a/grudge/symbolic/mappers/__init__.py +++ b/grudge/symbolic/mappers/__init__.py @@ -40,6 +40,7 @@ from pymbolic.mapper import CSECachingMapperMixin from grudge import sym import grudge.symbolic.operators as op +from grudge.tools import OrderedSet # {{{ mixins @@ -1216,13 +1217,17 @@ class ErrorChecker(CSECachingMapperMixin, IdentityMapper): # {{{ collectors for various symbolic operator components +# To maintain deterministic output in code generation, these collectors return +# OrderedSets. (The order of collected values determines the names of variables, +# which are significant to loopy.) + class CollectorMixin(OperatorReducerMixin, LocalOpReducerMixin, FluxOpReducerMixin): def combine(self, values): from pytools import flatten - return set(flatten(values)) + return OrderedSet(flatten(values)) def map_constant(self, expr, *args, **kwargs): - return set() + return OrderedSet() map_grudge_variable = map_constant map_c_function = map_grudge_variable @@ -1247,9 +1252,9 @@ class BoundOperatorCollector(CSECachingMapperMixin, CollectorMixin, CombineMappe def map_operator_binding(self, expr): if isinstance(expr.op, self.op_class): - result = set([expr]) + result = OrderedSet([expr]) else: - result = set() + result = OrderedSet() return result | CombineMapper.map_operator_binding(self, expr) @@ -1259,7 +1264,7 @@ class FluxExchangeCollector(CSECachingMapperMixin, CollectorMixin, CombineMapper CombineMapper.map_common_subexpression def map_flux_exchange(self, expr): - return set([expr]) + return OrderedSet([expr]) # }}} diff --git a/grudge/tools.py b/grudge/tools.py index 88d67143..8a0f5e81 100644 --- a/grudge/tools.py +++ b/grudge/tools.py @@ -124,3 +124,76 @@ def partial_to_all_subset_indices(subsets, base=0): idx += 1 yield np.array(result, dtype=np.intp) + + +# {{{ OrderedSet + +# Source: http://code.activestate.com/recipes/576694-orderedset/ +# Author: Raymond Hettinger +# License: MIT + +try: + from collections.abc import MutableSet +except ImportError: + from collections import MutableSet + + +class OrderedSet(MutableSet): + + def __init__(self, iterable=None): + self.end = end = [] + end += [None, end, end] # sentinel node for doubly linked list + self.map = {} # key --> [key, prev, next] + if iterable is not None: + self |= iterable + + def __len__(self): + return len(self.map) + + def __contains__(self, key): + return key in self.map + + def add(self, key): + if key not in self.map: + end = self.end + curr = end[1] + curr[2] = end[1] = self.map[key] = [key, curr, end] + + def discard(self, key): + if key in self.map: + key, prev, next = self.map.pop(key) + prev[2] = next + next[1] = prev + + def __iter__(self): + end = self.end + curr = end[2] + while curr is not end: + yield curr[0] + curr = curr[2] + + def __reversed__(self): + end = self.end + curr = end[1] + while curr is not end: + yield curr[0] + curr = curr[1] + + def pop(self, last=True): + if not self: + raise KeyError('set is empty') + key = self.end[1][0] if last else self.end[2][0] + self.discard(key) + return key + + def __repr__(self): + if not self: + return '%s()' % (self.__class__.__name__,) + return '%s(%r)' % (self.__class__.__name__, list(self)) + + def __eq__(self, other): + if isinstance(other, OrderedSet): + return len(self) == len(other) and list(self) == list(other) + return set(self) == set(other) + +# }}} -- GitLab From 9c9092a4cdf044ce663f74be44b6bcba6414046b Mon Sep 17 00:00:00 2001 From: Matt Wala Date: Tue, 9 Apr 2019 04:31:37 +0200 Subject: [PATCH 2/3] Clarify description --- grudge/symbolic/mappers/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/grudge/symbolic/mappers/__init__.py b/grudge/symbolic/mappers/__init__.py index e47bd6c2..8b9bf73b 100644 --- a/grudge/symbolic/mappers/__init__.py +++ b/grudge/symbolic/mappers/__init__.py @@ -1218,8 +1218,10 @@ class ErrorChecker(CSECachingMapperMixin, IdentityMapper): # {{{ collectors for various symbolic operator components # To maintain deterministic output in code generation, these collectors return -# OrderedSets. (The order of collected values determines the names of variables, -# which are significant to loopy.) +# OrderedSets. (As an example for why this is useful, the order of collected +# values determines the names of intermediate variables. If variable names +# aren't deterministic, loopy's kernel caching doesn't help us much across +# runs.) class CollectorMixin(OperatorReducerMixin, LocalOpReducerMixin, FluxOpReducerMixin): def combine(self, values): -- GitLab From 54bb7d7f4b63a11a1d23e0dcc8e2b7240be3b218 Mon Sep 17 00:00:00 2001 From: Matt Wala Date: Wed, 10 Apr 2019 17:08:25 -0500 Subject: [PATCH 3/3] Add a test of sorts --- test/test_grudge.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/test_grudge.py b/test/test_grudge.py index cf820f4c..97c5a012 100644 --- a/test/test_grudge.py +++ b/test/test_grudge.py @@ -485,6 +485,31 @@ def test_foreign_points(ctx_factory): bind(pdiscr, sym.nodes(dim)**2)(queue) +def test_op_collector_order_determinism(): + class TestOperator(sym.Operator): + + def __init__(self): + sym.Operator.__init__(self, sym.DD_VOLUME, sym.DD_VOLUME) + + mapper_method = "map_test_operator" + + from grudge.symbolic.mappers import BoundOperatorCollector + + class TestBoundOperatorCollector(BoundOperatorCollector): + + def map_test_operator(self, expr): + return self.map_operator(expr) + + v0 = sym.var("v0") + ob0 = sym.OperatorBinding(TestOperator(), v0) + + v1 = sym.var("v1") + ob1 = sym.OperatorBinding(TestOperator(), v1) + + # The output order isn't significant, but it should always be the same. + assert list(TestBoundOperatorCollector(TestOperator)(ob0 + ob1)) == [ob0, ob1] + + def test_bessel(ctx_factory): cl_ctx = cl.create_some_context() queue = cl.CommandQueue(cl_ctx) -- GitLab