compiler: Revamp CIRE exploiting EvalDerivative #1688

FabioLuporini · 2021-05-03T13:46:20Z

By searching and optimizing EvalDerivatives, that is sub-expressions produced by the evaluation of Derivatives, the CIRE pass is now more rigorous, robust, effective, and quick. It also opens up the possibility of easily synthesizing different variants of the same code.

The PR is equipped with many new tests, and adjust all of the existing ones to the new CIRE infrastructure.

Some optimization options , such as cire-mincost-* and cire-repeats-* have finally been dropped and replaced by two simple knobs, namely cire-mingain and cire-schedule, which are suitably documented

EDIT: I will also leave some inline comments for ease of reviewing

Note 1: AFAICT, there is no compilation speed regression

Note 2: this PR may impact the code generated by the compiler for some Operators. In all my experiments, I have not seen even a single regression in execution time.

So, Note 1 + Note 2 => all sort of timings should be improving. It'd be great to have TheMatrix to easily test this on multiple architectures simultaneously, but for now we'll need some beta testing I guess

review-notebook-app · 2021-05-03T13:46:24Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2021-05-03T13:49:33Z

Codecov Report

Merging #1688 (cadb4e1) into master (299b116) will increase coverage by 0.03%.
The diff coverage is 95.78%.

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
+ Coverage   86.68%   86.72%   +0.03%     
==========================================
  Files         218      218              
  Lines       33389    33712     +323     
  Branches     4342     4408      +66     
==========================================
+ Hits        28943    29236     +293     
- Misses       3959     3988      +29     
- Partials      487      488       +1

Impacted Files	Coverage Δ
devito/finite_differences/coefficients.py	`91.86% <ø> (ø)`
tests/test_gpu_common.py	`1.89% <0.00%> (-0.01%)`	⬇️
devito/symbolics/queries.py	`48.69% <8.69%> (-10.45%)`	⬇️
devito/ir/support/vector.py	`93.81% <33.33%> (-0.43%)`	⬇️
devito/operator/operator.py	`88.26% <85.71%> (-0.28%)`	⬇️
devito/passes/clusters/aliases.py	`97.44% <98.54%> (+0.37%)`	⬆️
devito/core/cpu.py	`100.00% <100.00%> (ø)`
devito/core/gpu.py	`94.68% <100.00%> (-0.03%)`	⬇️
devito/finite_differences/derivative.py	`86.41% <100.00%> (-0.17%)`	⬇️
devito/finite_differences/differentiable.py	`95.10% <100.00%> (-0.08%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 299b116...cadb4e1. Read the comment docs.

FabioLuporini · 2021-05-03T13:52:35Z

devito/core/cpu.py

        clusters = fuse(clusters)
-        clusters = eliminate_arrays(clusters)


Note for reviewers: yep, we can drop a whole pass, as thanks to the many generalisations it has become unnecessary (while being much weaker then what we have now)

By unecessary you mean that it is not anymore giving improvements?

FabioLuporini · 2021-05-03T13:53:16Z

devito/core/gpu.py

-        clusters = eliminate_arrays(clusters)
+
+        # Reduce flops
+        clusters = cse(clusters, sregistry)


Note for reviewers: note that now cse is performed after fuse. That's fine, it was wrong before

Why was it worng?

FabioLuporini · 2021-05-03T13:53:49Z

devito/finite_differences/derivative.py

@@ -359,8 +356,4 @@ def _eval_fd(self, expr):
        for e in self._ppsubs:
            res = res.xreplace(e)

-        # Step 5: Cast to EvaluatedDerivative
-        assert res.is_Add
-        res = EvalDerivative(*res.args, evaluate=False)


Note for reviewers: now unnecessary as done directly inside finite_difference.py

FabioLuporini · 2021-05-03T13:54:33Z

devito/ir/clusters/queue.py

@@ -117,24 +117,3 @@ def _fetch_properties(self, clusters, prefix):
            for i in prefix:
                properties[i.dim].update(v.get(i.dim, set()))
        return properties
-
-
-class Context(Queue):


Note for reviewers: dropping as unnecessary now

FabioLuporini · 2021-05-03T13:55:19Z

devito/passes/clusters/factorization.py

@@ -78,8 +78,11 @@ def collect_const(expr):
            # Back to the running example
            # -> (a + c)
            add = Add(*v)
-            # -> 3.*(a + c)
-            mul = Mul(k, add, evaluate=False)
+            if add == 0:


Note for reviewers: these are small bug fixes... without, in some corner circumstances we were generating expressions such as a + 0, while now we would generate just a

FabioLuporini · 2021-05-03T13:55:48Z

devito/passes/equations/linearity.py

@@ -185,7 +185,7 @@ def _(expr):

    derivs, others = split(args, lambda a: isinstance(a, sympy.Derivative))
    if not derivs:
-        return expr
+        return reuse_if_untouched(expr, args)


Note for reviewers: this is a performance-bug fix, and it's now tested

FabioLuporini · 2021-05-03T13:57:27Z

devito/passes/clusters/aliases.py

-                # E.g., `x0_blk0` or (`a[y_m+1]` => `y not in imapper`)
-                intervals.append(i)
-                continue
+                if i.dim in a.free_symbols:


Note for reviewers: this is a small bug fix

What does it fix? What is the bug?

I don't remember exactly anymore, but I've added a test. It had to do with hierarchical blocking and maybe nested parallelism

FabioLuporini · 2021-05-03T13:58:46Z

devito/passes/clusters/aliases.py

+    def _generate(self, exprs, exclude):
+        # E.g., extract `u.dx*a*b` and `u.dx*a*c` from `[(u.dx*a*b).dy`, `(u.dx*a*c).dy]`
+        def cbk_search(expr):
+            if isinstance(expr, EvalDerivative) and not expr.base.is_Function:


Note for reviewers: this is the game changer -- we can finally look for EvalDerivative, rather than speculatively searching all sort of expressions in sum-of-product form

mloubout

Some comments. looks good

mloubout · 2021-05-05T10:33:53Z

devito/core/cpu.py

@@ -24,17 +22,17 @@ class Cpu64OperatorMixin(object):
    3 => "blocks", "sub-blocks", and "sub-sub-blocks", ...
    """

-    CIRE_MINCOST_INV = 50
+    CIRE_MINGAIN = 10


Do we want backward compatibility?

CIRE_MINCOST_INV = 0

And still use it if set (or throw a warning).

good point, I'm going to deprecate stuff and raise warnings accordingly

mloubout · 2021-05-05T10:34:02Z

devito/core/cpu.py

    """

-    CIRE_MINCOST_SOPS = 10
+    CIRE_SCHEDULE = 'automatic'


Higher (lower) values
this higher lower seems a bit unclear to me

mloubout · 2021-05-05T10:35:36Z

devito/core/gpu.py

@@ -26,17 +25,17 @@ class DeviceOperatorMixin(object):
    3 => "blocks", "sub-blocks", and "sub-sub-blocks", ...
    """

-    CIRE_MINCOST_INV = 50
+    CIRE_MINGAIN = 10


same as cpu

mloubout · 2021-05-05T10:36:49Z

devito/core/gpu.py

-            'sops': oo.pop('cire-mincost-sops', cls.CIRE_MINCOST_SOPS)
-        }
+        o['cire-mingain'] = oo.pop('cire-mingain', cls.CIRE_MINGAIN)
+        o['cire-schedule'] = oo.pop('cire-schedule', cls.CIRE_SCHEDULE)


Wouldn't it be easier to have a an abstract operator class to avoid this code duplication in cpu and gpu?

I thought about it (obviously) but in reality there are quite a few differences between the two, so I'd rather keep it separated

devito/finite_differences/differentiable.py

mloubout · 2021-05-05T10:45:07Z

devito/finite_differences/differentiable.py

+        try:
+            obj.base = base
+        except AttributeError:
+            # This might happen if e.g. one attempts a (re)construction with


Actually there may be even weirder case. Like consider first order derivative with coefficient 1/h_x, 0 then it would evaluate to a Mul (1/h_x*u)which is not anAdd` so may break the whole constructor.

I tried, but I wasn't able to produce them, not sure why... any chance you can try writing a reproducer?

mloubout · 2021-05-05T10:47:31Z

devito/finite_differences/differentiable.py

+        obj = super().__new__(cls, *args, **kwargs)
+
+        try:
+            obj.base = base


what's the need for that one?

CIRE won't optimize EvalDerivatives if the base is a pure Function, such as u.dx, simply because there would be nothing to optimize. It needs to be an expression to be worth optimizing, ie expr.dx . CTRL+F for .base and you'll see where it's used in aliases.py

mloubout · 2021-05-05T10:53:56Z

devito/passes/clusters/aliases.py

+    # `score > M` => optimized
+    # `m <= score <= M` => maybe discarded, maybe optimized -- depends on heuristics
+    m = mingain
+    M = mingain*3


yes. Heuristics are in master too in this same function, though the "3" is new. It works perfectly for what we want and don't want to optimize given the other chosen values, what can I say..

mloubout · 2021-05-05T10:55:36Z

devito/passes/clusters/aliases.py

+    # TODO: Once we'll be able to keep Derivative intact down to this point,
+    # we won't probably need this function anymore, because, in essence, what
+    # this function is doing is reconstructing prematurely lowered information
+    grids = {getattr(i.function, 'grid', None) for i in expr.free_symbols} - {None}


why is this done here? And why not expr.grid

georgebisbas

Will shortly do another pass

georgebisbas · 2021-05-05T12:31:40Z

devito/core/cpu.py

        clusters = fuse(clusters)
-        clusters = eliminate_arrays(clusters)


By unecessary you mean that it is not anymore giving improvements?

georgebisbas · 2021-05-05T12:32:24Z

devito/core/gpu.py

-            'sops': oo.pop('cire-mincost-sops', cls.CIRE_MINCOST_SOPS)
-        }
+        o['cire-mingain'] = oo.pop('cire-mingain', cls.CIRE_MINGAIN)
+        o['cire-schedule'] = oo.pop('cire-schedule', cls.CIRE_SCHEDULE)


georgebisbas · 2021-05-05T12:32:46Z

devito/core/gpu.py

-        clusters = eliminate_arrays(clusters)
+
+        # Reduce flops
+        clusters = cse(clusters, sregistry)


Why was it worng?

georgebisbas · 2021-05-05T12:41:13Z

devito/ir/clusters/cluster.py

@@ -164,7 +164,7 @@ def used_dimensions(self):
        example, reduction or redundant (i.e., invariant) Dimensions won't
        appear in an expression.
        """
-        return set().union(*[i._defines for i in self.free_symbols if i.is_Dimension])
+        return {i for i in self.free_symbols if i.is_Dimension}


How is that helping?

Out of curiosity

oh, now it adheres to the specification of the method (docstring)
before, it was mistakenly adding all dimensions, including the parent ones, which may actually not appear directly in the cluster

georgebisbas · 2021-05-05T13:05:56Z

devito/passes/clusters/aliases.py

        raise NotImplementedError

-    def _in_writeto(self, dim, cluster):
+    def _betterv(self, delta_flops, delta_ws):


name change?

deliver typo

agree, gonna find a better name, current one is leftover

georgebisbas · 2021-05-05T13:07:27Z

devito/passes/clusters/aliases.py


-        return mapper
+        # Rule out extractions that are *not* independent of the Dimension


are not independent \equivalent to are dependent (?)

indeed, will fix

georgebisbas · 2021-05-05T13:45:16Z

devito/passes/clusters/aliases.py

@@ -485,91 +543,91 @@ def collect(extracted, ispace, min_storage):
                distance = [(d, set(v)) for d, v in LabeledVector.transpose(*distance)]
                distances.append(LabeledVector([(d, v.pop()) for d, v in distance]))

-            aliases.add(alias, list(mapper), aliaseds, distances)
+            # Compute the alias score
+            na = len(aliaseds)


other var name?

georgebisbas · 2021-05-05T13:45:53Z

devito/passes/clusters/aliases.py

+    # `score > M` => optimized
+    # `m <= score <= M` => maybe discarded, maybe optimized -- depends on heuristics
+    m = mingain
+    M = mingain*3


georgebisbas · 2021-05-05T13:47:20Z

devito/passes/clusters/aliases.py

-                # E.g., `x0_blk0` or (`a[y_m+1]` => `y not in imapper`)
-                intervals.append(i)
-                continue
+                if i.dim in a.free_symbols:


What does it fix? What is the bug?

georgebisbas · 2021-05-05T13:53:53Z

devito/symbolics/manipulation.py

@@ -294,9 +294,10 @@ def subs_op_args(expr, args):
    return expr.subs({i.name: args[i.name] for i in expr.free_symbols if i.name in args})


-def rebuild_if_untouched(expr, args, evaluate=False):
+def reuse_if_untouched(expr, args, evaluate=False):


georgebisbas · 2021-05-06T07:27:43Z

devito/core/cpu.py

    """

-    CIRE_MINCOST_SOPS = 10
+    CIRE_SCHEDULE = 'automatic'


Higher (lower) values
this higher lower seems a bit unclear to me

georgebisbas · 2021-05-06T07:38:26Z

devito/ir/support/properties.py

@@ -81,3 +81,7 @@ def normalize_properties(*args):
        properties.update(p - drop)

    return properties
+
+
+def relax_properties(properties):


I think you mentioned that in the past? To drop properties?

georgebisbas · 2021-05-06T07:42:18Z

tests/test_dse.py

+        assert len(arrays) == 5
+        assert len(FindNodes(VExpanded).visit(op._func_table['bf0'])) == 3
+        xs, ys, zs = self.get_params(op, 'x0_blk0_size', 'y0_blk0_size', 'z_size')
+        # The three kind of derivatives taken -- in x, y, z -- all are over


three kinds?

georgebisbas · 2021-05-06T07:43:06Z

tests/test_dse.py


        # Also check against expected operation count to make sure
        # all redundancies have been detected correctly
-        assert summary[('section1', None)].ops == 80
+        assert summary1[('section1', None)].ops == 75


WHy summary1 instead of summary?

georgebisbas · 2021-05-06T07:44:19Z

tests/test_dse.py


 class TestTTIv2(object):

    @switchconfig(profiling='advanced')
    @pytest.mark.parametrize('space_order,expected', [
-        (4, 212), (12, 420)
+        (4, 191), (12, 383)


…ule_padding

mloubout · 2021-05-18T10:41:44Z

devito/operator/operator.py

@@ -918,6 +918,14 @@ def parse_kwargs(**kwargs):
    options.setdefault('mpi', configuration['mpi'])
    for k, v in configuration['opt-options'].items():
        options.setdefault(k, v)
+    # Handle deprecations
+    deprecated_options = ('cire-mincost-inv', 'cire-mincost-sops')


cire-maxalias as well

added commit already in follow-up PR

FabioLuporini requested a review from mloubout May 3, 2021 13:46

FabioLuporini requested review from georgebisbas and jkwashbourne May 3, 2021 13:50

FabioLuporini added the compiler label May 3, 2021

FabioLuporini commented May 3, 2021

View reviewed changes

FabioLuporini force-pushed the cire-ultimate-revamp branch from ee22a3e to 76d17bd Compare May 4, 2021 08:21

mloubout reviewed May 5, 2021

View reviewed changes

georgebisbas reviewed May 5, 2021

View reviewed changes

georgebisbas reviewed May 6, 2021

View reviewed changes

FabioLuporini force-pushed the cire-ultimate-revamp branch 2 times, most recently from 20d5870 to 68f8e0c Compare May 11, 2021 14:47

FabioLuporini added 10 commits May 12, 2021 10:40

compiler: Revamp CIRE

41f5eda

examples: Update performance notebook

6027a0e

tests: Check aliases extraction within conditionals

ba51d19

compiler: Drop unnecessary __new__ overrides

3f1eb42

compiler: Refactor and improve EvalDerivative

05ff539

compiler: Revamp CIRE

0d38bfc

compiler: Patch count()

0fad12f

compiler: Remove cire-maxalias reminiscenses

b765021

compiler: Tweak EvalDerivative hierarchy

f99fb6b

compiler: Revamp CIRE

60fa2a1

FabioLuporini added 26 commits May 12, 2021 10:40

compiler: Revamp CIRE

e11f526

compiler: Avoid a + 0 in corner factorization cases

1761daa

compiler: Patch Cluster's DataSpace.union after CIRE's optimize_sched…

076bc8f

…ule_padding

compiler: Revamp CIRE

6bccd04

compiler: Patch collect_derivatives

24c9d63

compiler: Use SymPy's _mulsort and _addsort, for homogeneity

b004148

tests: Adjut expected output in test_derivatives

9b2a32c

tests: Update expected docstring output

f87d46f

examples: Update expected output in cfd notebooks

b5b8630

tests: Update test_dle for retrocompatibility

605d183

tests: Update test_mpi for retrocompatibility

9478fd6

examples: Update expected output in seismic notebooks

437d4ff

examples: Update performance notebooks

428a218

tests: Update test_symbolic_coefficients

d94ea89

compiler: rebuild-if-untouched -> reuse-if-untouched

4f25a16

compiler: Improve CireSops (impacting tti_adjoint)

30276f8

compiler: Tweak CIRE score calculation

6dfb790

tests: Check operation count in tti adjoint

39d3643

examples: Update userapi notebooks

a01f44d

compiler: Patch CIRE's maxpar option

81f1135

tests: Relax test_ftemps_option tolerance

29100de

compiler: Handle deprecated optimization options

c68cc44

compiler: Polish revamped CIRE

1b8931d

compiler: Split processing of elementaries and divs in CIRE

86d450a

compiler: Flatten Mul's args if necessary

4f3cd32

compiler: Optimize the -1*-1*... case when building Mul

cadb4e1

FabioLuporini force-pushed the cire-ultimate-revamp branch from 951d5f6 to cadb4e1 Compare May 12, 2021 08:41

mloubout reviewed May 18, 2021

View reviewed changes

FabioLuporini merged commit 8d60c7b into master Jun 7, 2021

FabioLuporini deleted the cire-ultimate-revamp branch June 7, 2021 12:31

		clusters = fuse(clusters)
		clusters = eliminate_arrays(clusters)


		return mapper
		# Rule out extractions that are not independent of the Dimension

compiler: Revamp CIRE exploiting EvalDerivative #1688

compiler: Revamp CIRE exploiting EvalDerivative #1688

Conversation

FabioLuporini commented May 3, 2021 • edited Loading

review-notebook-app bot commented May 3, 2021

codecov bot commented May 3, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini commented May 3, 2021 •

edited

Loading

codecov bot commented May 3, 2021 •

edited

Loading

georgebisbas May 5, 2021 •

edited

Loading

georgebisbas May 5, 2021 •

edited

Loading