Analysis at a higher level of abstraction #1027

FabioLuporini · 2019-12-16T13:17:03Z

WARNING: this PR is based on top of #1026 , so it's pointless to look at the diff with master until #1026 will have been merged.

This PR moves several analysis passes from the IET level to the Cluster level.

No new functionality is added. This is in preparation for another PR.

georgebisbas

Needs rebasing, right?

georgebisbas · 2020-01-06T08:03:58Z

devito/ir/iet/nodes.py

-from devito.ir.iet import (IterationProperty, SEQUENTIAL, PARALLEL, PARALLEL_IF_ATOMIC,
-                           VECTOR, WRAPPABLE, ROUNDABLE, AFFINE, OVERLAPPABLE)
-from devito.ir.support import Forward, detect_io
+from devito.ir.support import (SEQUENTIAL, PARALLEL, PARALLEL_IF_ATOMIC, VECTORIZED,


vectorizable?

georgebisbas · 2020-01-06T08:08:23Z

devito/ir/support/properties.py

+COLLAPSED = lambda i: Property('collapsed', i)
+"""Collapsing Dimensions."""
+
+VECTORIZED = Property('vector-dim')


as before.. vectorizable?

no, now it's really VECTORIZED

georgebisbas · 2020-01-06T09:04:36Z

devito/operator/profiling.py

@@ -104,7 +104,7 @@ def timer_on(self, name, comm=None):
            If provided, the global execution time is derived by a single MPI
            rank, with timers started and stopped right after an MPI barrier.
        """
-        if comm is not MPI.COMM_NULL:
+        if comm and comm is not MPI.COMM_NULL:


This should have been spotted earlier on, right?

mloubout · 2020-01-06T12:45:53Z

devito/ir/iet/nodes.py

@@ -366,7 +366,7 @@ class Iteration(Node):
        The for-loop direction. Accepted:
        - ``Forward``: i += stepping (defaults)
        - ``Backward``: i -= stepping
-    properties : IterationProperty or list of IterationProperty, optional
+    properties : Property or list of Property, optional


I get the idea, but "Property" feels very vague/general, maybe "CompilerProperty" or "PasssProperty" or just keep the previous IterationProperty

Would vote for IterationProperty

It is not an IterationProperty (anymore), otherwise I'd have kept the original name

mloubout · 2020-01-06T12:47:50Z

devito/ir/support/properties.py

+from devito.tools import Tag
+
+
+class Property(Tag):


As above, don't really like the name.

mloubout · 2020-01-06T12:48:59Z

devito/ir/support/properties.py

+"""
+A modulo-N Dimension (i.e., cycling over i, i+1, i+2, ..., i+N-1) that could
+safely be turned into a modulo-K Dimension, with K < N. For example:
+u[t+1, ...] = f(u[t, ...], u[t-1, ...]) --> u[t-1, ...] = f(u[t, ...], u[t-1, ...]).


that's t-1 on both side of = in the second part.

yes, that's the point. A very special case in which you could get away with two timeslots instead of three (here it's t-1 and t, could have been t+1 and t)

If it's inside f it's note the case as it implies it's not just a overwrite but need to know t-1 everywhere.

mloubout · 2020-01-06T12:51:07Z

tests/test_autotuner.py

@@ -96,7 +96,7 @@ def test_mode_destructive():
    grid = Grid(shape=(96, 96, 96))
    f = TimeFunction(name='f', grid=grid, time_order=0)

-    op = Operator(Eq(f, f + 1.), dle=('advanced', {'openmp': False}))
+    op = Operator(Eq(f.forward, f + 1.), dle=('advanced', {'openmp': False}))


I think it was just a leftover. I have reinstated the origin version

mloubout · 2020-01-06T12:53:37Z

tests/test_operator.py

@@ -498,7 +498,7 @@ def test_default_functions(self):
        grid = Grid(shape=(5, 6, 7))
        f = TimeFunction(name='f', grid=grid)
        g = Function(name='g', grid=grid)
-        op = Operator(Eq(g, g + f), dle=('advanced', {'openmp': False}))
+        op = Operator(Eq(f.forward, g + f), dle=('advanced', {'openmp': False}))


f would have worked too.

But not g. With g on the LHS, this is a fully parallel loop nest -- you may go in parallel even over time -- and heuristically the compiler doesn't do blocking if there is not at least one SEQUENTIAL Iteration

the test however expects x0_blk_size, ie it expects blocking, so...

mloubout · 2020-01-06T12:55:47Z

devito/ir/clusters/cluster.py

        """
-        return Cluster(exprs, self.ispace, self.dspace, self.guards)
+        # Shortcut for backwards compatibility


Maybe raise a warning if this is only there for backward compatibility.

oh no this is just an internal thing

and in fact, no worries -- this is changing again in one of the subsequent PRs (getting a proper cleanup)

FabioLuporini

Addressed your comments + rebased on latest master

I'll stick to Property for now, but I'll think of a better name before landing the next PR. Though to be fair it's not ambiguous, so it seems OK to me

FabioLuporini · 2020-01-07T08:13:38Z

tests/test_autotuner.py

@@ -96,7 +96,7 @@ def test_mode_destructive():
    grid = Grid(shape=(96, 96, 96))
    f = TimeFunction(name='f', grid=grid, time_order=0)

-    op = Operator(Eq(f, f + 1.), dle=('advanced', {'openmp': False}))
+    op = Operator(Eq(f.forward, f + 1.), dle=('advanced', {'openmp': False}))


I think it was just a leftover. I have reinstated the origin version

FabioLuporini · 2020-01-07T08:19:11Z

tests/test_operator.py

@@ -498,7 +498,7 @@ def test_default_functions(self):
        grid = Grid(shape=(5, 6, 7))
        f = TimeFunction(name='f', grid=grid)
        g = Function(name='g', grid=grid)
-        op = Operator(Eq(g, g + f), dle=('advanced', {'openmp': False}))
+        op = Operator(Eq(f.forward, g + f), dle=('advanced', {'openmp': False}))


f would have worked too.

But not g. With g on the LHS, this is a fully parallel loop nest -- you may go in parallel even over time -- and heuristically the compiler doesn't do blocking if there is not at least one SEQUENTIAL Iteration

the test however expects x0_blk_size, ie it expects blocking, so...

FabioLuporini · 2020-01-07T08:22:04Z

devito/ir/clusters/cluster.py

        """
-        return Cluster(exprs, self.ispace, self.dspace, self.guards)
+        # Shortcut for backwards compatibility


oh no this is just an internal thing

and in fact, no worries -- this is changing again in one of the subsequent PRs (getting a proper cleanup)

codecov · 2020-01-07T08:24:23Z

Codecov Report

Merging #1027 into master will increase coverage by 0.01%.
The diff coverage is 96.35%.

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   88.71%   88.73%   +0.01%     
==========================================
  Files         116      118       +2     
  Lines       14715    14742      +27     
  Branches     2722     2710      -12     
==========================================
+ Hits        13055    13081      +26     
- Misses       1315     1316       +1     
  Partials      345      345

Impacted Files	Coverage Δ
devito/ir/iet/scheduler.py	`93.61% <ø> (ø)`	⬆️
devito/ir/iet/__init__.py	`100% <ø> (ø)`	⬆️
devito/mpi/routines.py	`92.91% <100%> (+0.01%)`	⬆️
devito/ir/stree/algorithms.py	`100% <100%> (ø)`	⬆️
devito/ir/clusters/__init__.py	`100% <100%> (ø)`	⬆️
devito/ir/support/__init__.py	`100% <100%> (ø)`	⬆️
devito/ir/support/properties.py	`100% <100%> (ø)`
devito/ir/stree/tree.py	`87.77% <100%> (+0.13%)`	⬆️
devito/ir/iet/analysis.py	`100% <100%> (+5.58%)`	⬆️
devito/dse/rewriters.py	`94.11% <100%> (ø)`	⬆️
... and 16 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 7c45f89...7e5d2f2. Read the comment docs.

This was referenced Dec 17, 2019

Generalize Targets as Operators #1028

Merged

Generalize DSE as compiler passes #1030

Merged

FabioLuporini force-pushed the hoist-analysis branch from 4aed90c to 4a8e197 Compare January 6, 2020 09:07

georgebisbas reviewed Jan 6, 2020

View reviewed changes

mloubout reviewed Jan 6, 2020

View reviewed changes

FabioLuporini commented Jan 7, 2020

View reviewed changes

FabioLuporini added 24 commits January 7, 2020 09:23

ir: Move Property from /iet to /support

3538f83

ir: Prepare Clusters to carry Properties

14434f5

ir: Add FDTA/FATD processing modes for cluster optimization

86caa5a

ir: Move clustering Queue to processing.py

da5af2e

ir: Fixup FATD mode for Cluster processing

e7c3edc

ir: Clustering's processing.py -> queue.py

fd8986e

ir: Add machinery to detect Cluster properties

a7c4948

ir: Plumb properties from Clusters down to IET

3326539

ir: Generalize Cluster.rebuild

de0a34e

ir: Clean up docstrings

c220d07

ir: Hoist make_iteration_parallel

2c00af4

ir: Move Cluster analysis inside optimise

1316b9a

ir: Collect properties by Cluster groups

cbf6e46

ir: Drop useless mark_iterations_vectorizable

2258c2c

ir: Hoist mark_iteration_wrappable

b06d8da

ir: Hoist mark_iteration_roundable

07923ed

ir: Hoist mark_iteration_affine

00d13d5

ir: Hoist mark_iteration_tilable

668ca76

ir: _make_scope -> _fetch_scope

8572c19

ir: Fix detection of cluster properties

0402ef5

ir: Refactor clusters analysis

bc8e4bd

targets: Do not ompize loops not in canonical form

ca369ba

examples: Update compiler notebooks

cb30a39

tests: Reinstante previous test_mode_destructive

7e5d2f2

FabioLuporini force-pushed the hoist-analysis branch from 4a8e197 to 7e5d2f2 Compare January 7, 2020 08:24

FabioLuporini merged commit ea661f9 into master Jan 7, 2020

FabioLuporini deleted the hoist-analysis branch January 7, 2020 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Analysis at a higher level of abstraction #1027

Analysis at a higher level of abstraction #1027

FabioLuporini commented Dec 16, 2019

georgebisbas left a comment

georgebisbas Jan 6, 2020

FabioLuporini Jan 6, 2020

georgebisbas Jan 6, 2020

FabioLuporini Jan 6, 2020

georgebisbas Jan 6, 2020

mloubout Jan 6, 2020

georgebisbas Jan 6, 2020

FabioLuporini Jan 7, 2020

mloubout Jan 6, 2020

mloubout Jan 6, 2020

FabioLuporini Jan 7, 2020

mloubout Jan 7, 2020

mloubout Jan 6, 2020

FabioLuporini Jan 7, 2020

mloubout Jan 6, 2020

FabioLuporini Jan 7, 2020

mloubout Jan 6, 2020

FabioLuporini Jan 7, 2020

FabioLuporini left a comment

FabioLuporini Jan 7, 2020

FabioLuporini Jan 7, 2020

FabioLuporini Jan 7, 2020

codecov bot commented Jan 7, 2020 •

edited

Analysis at a higher level of abstraction #1027

Analysis at a higher level of abstraction #1027

Conversation

FabioLuporini commented Dec 16, 2019

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

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 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

codecov bot commented Jan 7, 2020 • edited

Codecov Report

codecov bot commented Jan 7, 2020 •

edited