compiler: Refactorings, simplifications, generalizations #1810

FabioLuporini · 2021-12-16T15:49:48Z

As the title says, zero new features introduced by this PR.

The main changes are the following:

The DataSpace construction is now vastly easier than in master. Also, it is performed only once, when all Clusters will have been constructed and optimized. This makes the code: easier to understand and faster to execute
Guards (ie conditionals) are now a first-class citizen in the compiler. See ir/support/guards.py
A lot of dead code (also as a consequence of the aforementioned changes) was dropped (in particular, most of the Stencil class from ir/support/stencil.py , most of DataSpace, ...)
The Dimension hierarchy was refactored. Now there's IncrDimension and BlockDimension, the latter for loop blocking. They inherit from the same class, which contains most of the logic, AbstractIncrDimension. They behave the same! But explicit distinction helps with making the code neater in more than one place. Also, the is_SubIterator class property could be introduced, which introduces a clear separation between Dimensions that cannot create loops (the SubIterators...) and the other ones (like Dimension, SubDimension, CustomDimension)

I will also add comments to the code to guide the reviewing

FabioLuporini · 2021-12-16T15:50:13Z

devito/arch/archinfo.py

-
-    def lshw_gpu_info(text):
-        def lshw_single_gpu_info(text):
+    def homogenise_gpus(gpu_infos):


here u see a big diff, but it's mostly due to indentation...

FabioLuporini · 2021-12-16T15:50:28Z

devito/arch/archinfo.py

+
+        gpu_info = homogenise_gpus(gpu_infos)
+
+        # Also attach callbacks to retrieve instantaneous memory info


... this is really the only new part

devito/passes/clusters/asynchrony.py

devito/passes/iet/parpragma.py

FabioLuporini · 2021-12-16T15:52:58Z

devito/types/array.py

-        the Devito compiler) to ensure data coherence. 'uniform', instead,
-        means the Array will be used as if the underlying node had uniform
-        memory address space.
+        The memory space. Allowed values: 'local', 'mapped'.  Defaults to 'local'.


simplifcations...

FabioLuporini · 2021-12-16T15:53:32Z

devito/types/dimension.py

@@ -790,76 +795,9 @@ def free_symbols(self):
    _pickle_kwargs = DerivedDimension._pickle_kwargs + ['factor', 'condition', 'indirect']


-class SteppingDimension(DerivedDimension):


this was not dropped -- just moved below towards the bottom of the file!

codecov · 2021-12-16T15:54:09Z

Codecov Report

Merging #1810 (9ce8cb6) into master (3c89f70) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #1810      +/-   ##
==========================================
- Coverage   89.60%   89.57%   -0.04%     
==========================================
  Files         209      209              
  Lines       34307    34379      +72     
  Branches     5198     5197       -1     
==========================================
+ Hits        30741    30794      +53     
- Misses       3077     3097      +20     
+ Partials      489      488       -1

Impacted Files	Coverage Δ
devito/arch/compiler.py	`57.41% <ø> (ø)`
devito/passes/iet/orchestration.py	`60.11% <0.00%> (ø)`
devito/symbolics/extended_sympy.py	`96.01% <0.00%> (-0.72%)`	⬇️
tests/test_gpu_common.py	`1.48% <0.00%> (-0.06%)`	⬇️
devito/arch/archinfo.py	`47.32% <16.66%> (-1.83%)`	⬇️
devito/core/arm.py	`50.00% <50.00%> (+5.17%)`	⬆️
devito/core/operator.py	`87.59% <50.00%> (+0.77%)`	⬆️
devito/passes/clusters/asynchrony.py	`37.33% <50.00%> (+2.35%)`	⬆️
devito/logger.py	`77.77% <60.00%> (-2.62%)`	⬇️
devito/ir/support/guards.py	`70.23% <70.23%> (ø)`
... and 48 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 3c89f70...9ce8cb6. Read the comment docs.

mloubout

Looks like a nice cleanup. Will make a full pass over the next few days.

georgebisbas

A zero-level review. Will look properly next days. Interesting work done!

devito/ir/clusters/cluster.py

devito/ir/equations/equation.py

georgebisbas · 2021-12-16T16:03:03Z

devito/ir/equations/equation.py

+            if d.is_SubIterator:
+                iterators.setdefault(d.root, set()).add(d)
+            elif d.is_Conditional:
+                # Use `parent`, and `root`, because a ConditionalDimenion may


devito/ir/equations/equation.py

georgebisbas · 2021-12-16T16:03:50Z

devito/ir/equations/equation.py


    There are two main differences between a LoweredEq and a
    ClusterizedEq:

-    * In a ClusterizedEq, the iteration and data spaces must *always*
-      be provided by the caller.
+    * In a ClusterizedEq, the iteration space must *always* be provided


the IterationSpace

georgebisbas · 2021-12-16T16:11:24Z

devito/passes/iet/languages/openacc.py


-        collapsable = self._find_collapsable(root, candidates)
+        root, collapsable = self._select_candidates(candidates)


hmm....maybe better before...

devito/passes/iet/parpragma.py

georgebisbas · 2021-12-16T16:14:22Z

devito/types/dimension.py

@@ -36,13 +36,11 @@ class Dimension(ArgProvider):
                                           |
                              ---------------------------
                              |                         |
-                       BasicDimension            DefaultDimension
+                       DerivedDimension            DefaultDimension


maybe add all DImension subclasses to this plot?

as the upper part of the docstring says, just the API classes

tests/test_dle.py

georgebisbas · 2021-12-16T16:16:15Z

tests/test_dse.py

@@ -2565,11 +2572,11 @@ def test_fullopt(self):

        # Check expected opcount/oi
        assert summary[('section1', None)].ops == 90
-        assert np.isclose(summary[('section1', None)].oi, 1.511, atol=0.001)
+        assert np.isclose(summary[('section1', None)].oi, 2.031, atol=0.001)


byproduct of a more accurate dspace calculation

seems way.. more accurate..

georgebisbas · 2021-12-17T09:57:39Z

devito/arch/archinfo.py

-        def lshw_single_gpu_info(text):
+    def homogenise_gpus(gpu_infos):
+        """
+        Run homogeneity checks on a list of GPU, return GPU with count if


list of GPUs

georgebisbas · 2021-12-17T09:59:40Z

devito/core/autotuning.py

@@ -56,11 +56,15 @@ def autotune(operator, args, level, mode):
    if mode in ['preemptive', 'destructive']:
        for p in operator.parameters:
            if isinstance(p, MPINeighborhood):
-                at_args.update(MPINeighborhood(p.neighborhood)._arg_values())
+                at_args.update(


needed this change?

it's just to make the code more homogeneous. The line below had to be split over multiple lines due to the additions of the args argument, so I did the same aesthetic change here

review-notebook-app · 2021-12-20T09:41:13Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

devito/passes/iet/mpi.py

examples/performance/00_overview.ipynb

FabioLuporini

pushing revisions now

FabioLuporini · 2021-12-22T08:19:46Z

devito/arch/archinfo.py

-        def lshw_single_gpu_info(text):
+    def homogenise_gpus(gpu_infos):
+        """
+        Run homogeneity checks on a list of GPU, return GPU with count if


FabioLuporini · 2021-12-22T08:22:15Z

devito/core/autotuning.py

@@ -56,11 +56,15 @@ def autotune(operator, args, level, mode):
    if mode in ['preemptive', 'destructive']:
        for p in operator.parameters:
            if isinstance(p, MPINeighborhood):
-                at_args.update(MPINeighborhood(p.neighborhood)._arg_values())
+                at_args.update(


it's just to make the code more homogeneous. The line below had to be split over multiple lines due to the additions of the args argument, so I did the same aesthetic change here

devito/ir/clusters/cluster.py

FabioLuporini · 2021-12-22T08:23:35Z

devito/ir/equations/equation.py

+            if d.is_SubIterator:
+                iterators.setdefault(d.root, set()).add(d)
+            elif d.is_Conditional:
+                # Use `parent`, and `root`, because a ConditionalDimenion may


FabioLuporini · 2021-12-22T08:23:43Z

devito/ir/equations/equation.py


    There are two main differences between a LoweredEq and a
    ClusterizedEq:

-    * In a ClusterizedEq, the iteration and data spaces must *always*
-      be provided by the caller.
+    * In a ClusterizedEq, the iteration space must *always* be provided


FabioLuporini · 2021-12-22T08:31:55Z

devito/types/dimension.py

@@ -36,13 +36,11 @@ class Dimension(ArgProvider):
                                           |
                              ---------------------------
                              |                         |
-                       BasicDimension            DefaultDimension
+                       DerivedDimension            DefaultDimension


as the upper part of the docstring says, just the API classes

FabioLuporini · 2021-12-22T08:32:45Z

tests/test_dse.py

@@ -2565,11 +2572,11 @@ def test_fullopt(self):

        # Check expected opcount/oi
        assert summary[('section1', None)].ops == 90
-        assert np.isclose(summary[('section1', None)].oi, 1.511, atol=0.001)
+        assert np.isclose(summary[('section1', None)].oi, 2.031, atol=0.001)


byproduct of a more accurate dspace calculation

…itions

georgebisbas · 2021-12-23T09:52:03Z

devito/ir/clusters/cluster.py

@@ -182,6 +173,14 @@ def has_increments(self):
    def is_scalar(self):
        return not any(f.is_Function for f in self.scope.writes)

+    @cached_property
+    def grid(self):


I wonder whether this could help with some bugs that we have seen in the past..... need to search ...

georgebisbas · 2021-12-23T10:14:38Z

devito/logger.py

@@ -47,9 +47,19 @@
 }


+def _set_log_level(level):


Do we need both _set_log_level, set_log_level ?

georgebisbas · 2021-12-23T11:23:36Z

tests/test_dle.py

@@ -792,9 +800,9 @@ def test_nested_cache_blocking_structure_subdims(self, blocklevels):
    @pytest.mark.parametrize('exprs,collapsed,scheduling', [


So we are OK with this? Maybe we can drop some of them since there is no diff now.

georgebisbas · 2021-12-23T12:03:14Z

tests/test_dse.py

@@ -2565,11 +2572,11 @@ def test_fullopt(self):

        # Check expected opcount/oi
        assert summary[('section1', None)].ops == 90
-        assert np.isclose(summary[('section1', None)].oi, 1.511, atol=0.001)
+        assert np.isclose(summary[('section1', None)].oi, 2.031, atol=0.001)


seems way.. more accurate..

FabioLuporini added the compiler label Dec 16, 2021

FabioLuporini requested review from mloubout and georgebisbas December 16, 2021 15:49

FabioLuporini commented Dec 16, 2021

View reviewed changes

mloubout reviewed Dec 16, 2021

View reviewed changes

georgebisbas reviewed Dec 16, 2021

View reviewed changes

georgebisbas reviewed Dec 17, 2021

View reviewed changes

FabioLuporini force-pushed the tweaks-101010 branch from 52994a2 to d9ae5f1 Compare December 20, 2021 09:41

FabioLuporini force-pushed the tweaks-101010 branch 2 times, most recently from 79b00a2 to 45e6e23 Compare December 20, 2021 16:09

georgebisbas reviewed Dec 20, 2021

View reviewed changes

devito/passes/iet/mpi.py Show resolved Hide resolved

examples/performance/00_overview.ipynb Show resolved Hide resolved

examples/performance/00_overview.ipynb Show resolved Hide resolved

FabioLuporini commented Dec 22, 2021

View reviewed changes

FabioLuporini force-pushed the tweaks-101010 branch from 93b1b26 to e9fbe13 Compare December 22, 2021 08:40

FabioLuporini mentioned this pull request Dec 23, 2021

Integer overflow with Linearize. #1771

Closed

FabioLuporini added 14 commits December 23, 2021 10:41

compiler: Add symbolic SizeOf function

9d09a65

compiler: Patch arguments forwarding

2e56779

api: Add Function.nbytes

89d4570

compiler: _arg_as_ctype -> _arg_finalize

4e2dd09

arch: Extend autodetection machinery

0c6c1b3

misc: Add ReducerMap.reduce_inplace

38689ae

compiler: Rework arguments processing

a75736e

arch: Add memtotal and memavail

3078948

compiler: _mem_default -> _mem_local

2a5d24a

compiler: Promote _mem_local/_mem_mapped to AbstractFunction

e144138

compiler: Use ConditionFactor to identify factor-based conditionals

8d2f71c

compiler: Enhance Streaming to recognize composite, factor-based cond…

e88c69b

…itions

compiler: Make Streaming dispatch to instance methods

28bee96

compiler: Purge Streaming

f4bb9a8

FabioLuporini added 20 commits December 23, 2021 10:41

compiler: Refactor guarding

6251edd

compiler: Refactor IterationSpace and DataSpace derivation

2e7ef43

compiler: Tweak IntervalGroup.promote

4061a75

compiler: Rework DataSpace derivation

621f355

compiler: Polish dspace derivation

6dcc690

misc: flake8 happiness

75dd2c9

compiler: Drop dead code

44b7f57

compiler: Drop more dead code

011ad40

compiler: Improve parallelization heuristics

dc1b41b

compiler: Pass allocator through arguments

02c4528

compiler: Simplify streaming

18f42c1

compiler: Split IncrDim into AbsIncrDim, IncrDim, BlkDim

5d9c2f2

compiler: Fix openmp codegen

0000851

compiler: Turn optimize_halospots into an opt-option

922e24b

examples: Update expected output

ec7a013

mpi: Avoid generation of compute efuncs with diag2 mode

bfd5198

arch: Patch get_gpu_info for multiple devices

151af78

compiler: Tweaks aliases' iintervals

409f381

compiler: Track memused at arguments processing

0cb9b23

compiler: Use uint32 for linearization offsets, not int

314355a

FabioLuporini force-pushed the tweaks-101010 branch from b78623c to 97f6f30 Compare December 23, 2021 09:42

api: Patch set_log_level

7a4bfab

FabioLuporini force-pushed the tweaks-101010 branch from 97f6f30 to 7a4bfab Compare December 23, 2021 09:46

georgebisbas reviewed Dec 23, 2021

View reviewed changes

mloubout approved these changes Dec 23, 2021

View reviewed changes

FabioLuporini added 2 commits December 28, 2021 15:12

compiler: Patch Tasking to avoid unnecessary locks

ff385aa

api: Fixup Function.size

9ce8cb6

FabioLuporini merged commit 8e10af2 into master Dec 29, 2021

FabioLuporini deleted the tweaks-101010 branch December 29, 2021 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Refactorings, simplifications, generalizations #1810

compiler: Refactorings, simplifications, generalizations #1810

FabioLuporini commented Dec 16, 2021 •

edited

Loading

FabioLuporini Dec 16, 2021

FabioLuporini Dec 16, 2021

FabioLuporini Dec 16, 2021

FabioLuporini Dec 16, 2021

codecov bot commented Dec 16, 2021 •

edited

Loading

mloubout left a comment

georgebisbas left a comment

georgebisbas Dec 16, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 16, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 16, 2021

georgebisbas Dec 16, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 16, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 23, 2021

georgebisbas Dec 17, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 17, 2021

FabioLuporini Dec 22, 2021

review-notebook-app bot commented Dec 20, 2021

FabioLuporini left a comment

FabioLuporini Dec 22, 2021

FabioLuporini Dec 22, 2021

FabioLuporini Dec 22, 2021

FabioLuporini Dec 22, 2021

FabioLuporini Dec 22, 2021

FabioLuporini Dec 22, 2021

georgebisbas Dec 23, 2021

georgebisbas Dec 23, 2021

georgebisbas Dec 23, 2021

georgebisbas Dec 23, 2021


		gpu_info = homogenise_gpus(gpu_infos)

		# Also attach callbacks to retrieve instantaneous memory info

		@@ -790,76 +795,9 @@ def free_symbols(self):
		_pickle_kwargs = DerivedDimension._pickle_kwargs + ['factor', 'condition', 'indirect']


		class SteppingDimension(DerivedDimension):


		collapsable = self._find_collapsable(root, candidates)
		root, collapsable = self._select_candidates(candidates)

		@@ -792,9 +800,9 @@ def test_nested_cache_blocking_structure_subdims(self, blocklevels):
		@pytest.mark.parametrize('exprs,collapsed,scheduling', [

compiler: Refactorings, simplifications, generalizations #1810

compiler: Refactorings, simplifications, generalizations #1810

Conversation

FabioLuporini commented Dec 16, 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

codecov bot commented Dec 16, 2021 • edited Loading

Codecov Report

mloubout left a 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

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

review-notebook-app bot commented Dec 20, 2021

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

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 Dec 16, 2021 •

edited

Loading

codecov bot commented Dec 16, 2021 •

edited

Loading