compiler: Revamp compilation of halo exchanges #2089

FabioLuporini · 2023-03-16T14:58:39Z

This PR performs a massive re-engineering of how halo exchanges are detected, constructed, and lowered inside Devito

Why: in master, halo exchanges are detected in the stree layer, that is, after all clusters-level passes have been performed. The issue is that some of these passes "clutter" the equations/clusters (after all, we're lowering) by e.g. introducing temporaries and changing the index access functions. This has the effect of enormously complicating data dependence analysis. A complexity exploded when I added CUDA/HIP shared memory.

What: change of paradigm. Now detecting halo exchanges is one of the first things the compiler does, and it does that at the clusters level. A halo exchange is here represented simply as yet another Cluster. It could not have been metadata attached to Cluster because Clusters evolve during lowering, which would take us back to the problem we're trying to solve here. A "floating" Cluster is the way to go, in my opinion. I failed to see this mechanism for several years, until something flipped in my mind when I was at Rice.

How: significant code changes, but in the end, many are simplifications

This PR is not finished yet, there are a couple of small TODOs to deal with, but I'd like people to start reviewing because we'll have to merge it soon (there has been a demand for CUDA/HIP + MPI numbers for weeks...)

codecov · 2023-03-16T15:06:52Z

Codecov Report

Merging #2089 (536f560) into master (d0e705d) will decrease coverage by 0.08%.
The diff coverage is 80.21%.

@@            Coverage Diff             @@
##           master    #2089      +/-   ##
==========================================
- Coverage   87.80%   87.72%   -0.08%     
==========================================
  Files         221      221              
  Lines       38665    38951     +286     
  Branches     5000     5061      +61     
==========================================
+ Hits        33951    34171     +220     
- Misses       4164     4213      +49     
- Partials      550      567      +17

Impacted Files	Coverage Δ
devito/ir/support/syncs.py	`61.53% <0.00%> (ø)`
devito/symbolics/extended_sympy.py	`95.82% <ø> (-0.04%)`	⬇️
devito/types/parallel.py	`85.62% <ø> (ø)`
tests/test_dle.py	`96.27% <ø> (ø)`
devito/arch/compiler.py	`41.06% <4.34%> (-1.28%)`	⬇️
devito/passes/clusters/asynchrony.py	`11.64% <25.00%> (ø)`
devito/passes/iet/instrument.py	`72.72% <42.10%> (-16.47%)`	⬇️
devito/operator/operator.py	`89.13% <50.00%> (-1.53%)`	⬇️
devito/mpi/distributed.py	`90.23% <61.11%> (-1.88%)`	⬇️
devito/passes/clusters/blocking.py	`89.76% <65.90%> (-4.23%)`	⬇️
... and 23 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

georgebisbas

Will have a look end of next week

devito/ir/clusters/algorithms.py

georgebisbas · 2023-03-24T18:08:14Z

devito/ir/clusters/cluster.py

-                       guards=kwargs.get('guards', self.guards),
-                       properties=kwargs.get('properties', self.properties),
-                       syncs=kwargs.get('syncs', self.syncs))
+        return self.__class__(exprs=kwargs.get('exprs', self.exprs),


Why are you doing this?

reconstruction of potential Cluster subclasses

Should we ratehr have a _rebuild ?

well that's how the method is called :)

georgebisbas · 2023-03-24T18:12:29Z

devito/ir/stree/algorithms.py

@@ -1,75 +1,61 @@
+from collections import defaultdict
+from itertools import groupby
+


why empty line?

it's the standard adopted throughout the entirety of devito
...

@georgebisbas sorry about the three "..." above which I'm sure sounds a bit blunt -- but something must have gone south while github was (partly) down yesterday; probably it picked my "Comment" when it was still half baked

I had written something along the lines of:

<imports from stdling> <blankline> <imports from third parties> <blankline> <local import>

definitely not "..."

nw, my main fault is that I thought anytree was considered standard. I overall have the feeling that there should be files not following the first blankline but maybe I am wrong

probably, they might have escaped reviewing, fixable as we encounter them

georgebisbas · 2023-03-24T18:24:35Z

devito/operator/operator.py

-            gflopss = "%.2f GFlops/s" % fround(v.gflopss)
-            gpointss = "%.2f GPts/s" % fround(v.gpointss) if v.gpointss else None
-            metrics = ", ".join(i for i in [oi, gflopss, gpointss] if i is not None)
+            if v.gflopss:


When does this happen?

data streaming sections which only trigger data transfers (so they take up time) but no compute

mloubout · 2023-03-27T17:33:19Z

devito/ir/support/basic.py

@@ -23,6 +22,11 @@ class IndexMode(Tag):
 REGULAR = IndexMode('regular')
 IRREGULAR = IndexMode('irregular')

+Delta = Symbol(name='Δ')


I would refrain from using symbols with FD meaning as they may be used to define Function/....

OK replacing with ⋈

mloubout

Sorry took a bit to get back to it

Overall I think it's GTG, seem to be cleaning up a lot. Some minor comments only

mloubout · 2023-03-27T18:03:22Z

tests/test_mpi.py

+                         't,x0_blk0,y0_blk0,x,y,z,z')
+
+        def init(f, v=1):
+            f.data[:] = np.indices(grid.shape).sum(axis=0) % (.004*v) + .01


interesting choice

Are the numbers just random?

definitely not random

mloubout · 2023-03-27T18:04:57Z

devito/operator/operator.py

-            metrics = ", ".join(i for i in [oi, gflopss, gpointss] if i is not None)
+            if v.gflopss:
+                oi = "OI=%.2f" % fround(v.oi)
+                gflopss = "%.2f GFlops/s" % fround(v.gflopss)


I think someone asked for .3f somewhere in slack but not important

It was probably John, but I honestly prefer .2f. When would three digits be useful? Anyway, we can vote!

mloubout · 2023-03-27T18:07:10Z

devito/mpi/halo_scheme.py

+    __str__ = __repr__
+
+    def _sympystr(self, printer):
+        return str(self)


if you have a printer shouldn't it be printer.doprint(self) ?

I don't remember exactly, but probably not, pretty sure I tried it but didn't eventually obtained what I wanted. Maybe because the printer lacks the handle for self, that's why we end up here? Anyway, this is just for the repr within larger expressions

devito/mpi/distributed.py

mloubout · 2023-03-27T18:09:51Z

devito/arch/compiler.py

@@ -105,7 +105,7 @@ def sniff_mpi_distro(mpiexec):


 @memoized_func
-def sniff_mpi_flags():
+def sniff_mpi_flags(mpicc='mpicc'):


where is this used?

IIRC in the CUDA/HIP compiler classes

devito/ir/clusters/algorithms.py

georgebisbas · 2023-03-28T10:31:47Z

devito/ir/iet/efunc.py

-    return ElementalFunction(name, iet, retval, derive_parameters(iet), prefix,
-                             dynamic_parameters)
+    return ElementalFunction(name, iet, retval=retval,
+                             parameters=derive_parameters(iet), prefix=prefix,


I would lift this out of the call, probably cleaner? parameters=derive_parameters(iet),

georgebisbas · 2023-03-28T10:40:21Z

devito/passes/iet/engine.py

        DiscreteFunction: 1,
+        AbstractIncrDimension: 2,
+        BlockDimension: 3,
    }


Not sure I understood this; could you help with a brief explanation?

it's explained in the comment:

https://github.com/devitocodes/devito/blob/master/devito/passes/iet/engine.py#L240

if I have an object X that, among its fields, has a BlockDimension Y, then before attempting to reconstruct a new X I will need to have a reconstruction for Y, has "X depends on Y", in a logical sense

I remember I had a similar approach in my time-tiling, but was considered a kinda-hack :-)

Thankfully GitHub preserves history!

Can you link me to that discussion please?

It was a verbal/slack discussion AFAIR since I did not open this PR.
My point was not to bring it up this way.
AFAIR:
I wanted to bring in the concept of a new class of blocking dimensions that had additional min/max info for handling main/remainder areas. As in having the necessary info for that at cluster level.

Then in, IET-level, when relaxing, I would need to reconstruct both parent-child block dims as the min of child was dependent upon the main-remainder of father.

Long story short, I had the impression that we should not do sth like this. But I am fine.
At least what you do is clean/clear, mine was not really.

Are there other places in the framework where something similar is being done?

I'm not sure honestly, maybe? I mean, the concept of prioritizing processing is fairly widespread

georgebisbas · 2023-03-28T10:41:37Z

devito/passes/iet/engine.py

+    p = i.parent
+    pp = i.parent.parent
+
+    name0 = pp.name
+    base = sregistry.make_name(prefix=name0)
+    name1 = sregistry.make_name(prefix='%s_blk' % base)
+
+    bd = i.parent._rebuild(name1, pp)
+    d = i._rebuild(name0, bd, i._min.subs(p, bd), i._max.subs(p, bd))
+
+    mapper.update({
+        i: d,
+        i.parent: bd
+    })


This looks weird. Why is that happening?

Because if you reconstruct using i as "stamp", you better be sure to also replace all of its parent occurrences, or you end up with undefined symbols

georgebisbas · 2023-03-28T10:42:55Z

tests/test_dle.py

@@ -404,49 +404,6 @@ def test_cache_blocking_hierarchical(blockshape0, blockshape1, exception):

 @pytest.mark.parametrize("blockinner", [False, True])
 def test_cache_blocking_imperfect_nest(blockinner):
-    """


sort-of; now it's in test_mpi, though under a slightly different vest

Never mind, and thanks for making me notice: I had originally removed it because, yes, as I said, I was going to move it to test_mpi; but then I rolled most of the temporary edits back; hence it does make sense to keep this test where it was; so, reinstated.

georgebisbas · 2023-03-28T10:47:09Z

tests/test_mpi.py

+                         't,x0_blk0,y0_blk0,x,y,z,z')
+
+        def init(f, v=1):
+            f.data[:] = np.indices(grid.shape).sum(axis=0) % (.004*v) + .01


Are the numbers just random?

FabioLuporini · 2023-03-28T12:51:30Z

@mloubout @georgebisbas edits pushed, most comments addressed

@mloubout : yes, this PR indeed ships significant tidy up

FabioLuporini · 2023-03-28T12:54:46Z

devito/passes/clusters/misc.py

@@ -289,19 +302,10 @@ def is_cross(dep):
                    break

                # Any anti- and iaw-dependences impose that `cg1` follows `cg0`
-                # while not being its immediate successor (unless it already is),


btw, this is massive

georgebisbas

Not sure if I have more to add. I think I asked most of the things that I could not understand. I will approve, but happy to re-review if @FabioLuporini you think there are parts I should pay more attention

FabioLuporini added the compiler label Mar 16, 2023

FabioLuporini requested a review from mloubout March 16, 2023 14:58

georgebisbas reviewed Mar 17, 2023

View reviewed changes

devito/ir/clusters/algorithms.py Show resolved Hide resolved

FabioLuporini force-pushed the mpi-revamp branch 3 times, most recently from fa84374 to 0dc22be Compare March 21, 2023 15:16

FabioLuporini changed the title ~~Revamp compilation of halo exchanges~~ compiler: Revamp compilation of halo exchanges Mar 23, 2023

georgebisbas reviewed Mar 24, 2023

View reviewed changes

FabioLuporini force-pushed the mpi-revamp branch 2 times, most recently from 5b0009e to f71fc50 Compare March 27, 2023 09:02

mloubout reviewed Mar 27, 2023

View reviewed changes

georgebisbas reviewed Mar 28, 2023

View reviewed changes

FabioLuporini force-pushed the mpi-revamp branch from 1ddb0af to 78905e0 Compare March 28, 2023 12:50

FabioLuporini commented Mar 28, 2023

View reviewed changes

georgebisbas approved these changes Mar 28, 2023

View reviewed changes

FabioLuporini added 12 commits March 29, 2023 20:13

compiler: Sketch MPI dual mode

b4db16a

arch: Improve MPI+CUDA jit-compilation

25314b6

bench: Patch nosetup profiling with MPI

86f8cab

misc: Fix invalid escape sequence warning

342a84c

compiler: Revamp halo exchange compilation

533978c

compiler: Avoid redundant WaitLocks

35ce85c

compiler: Enhance abstract_efunc

93ab6b6

compiler: Add Properties.filter

7a3164b

compiler: Extirpate now obsolete fusion machinery

2d8506a

misc: Fix pep8 violations

2f6291f

compiler: Patch abstract_objects

583c8c7

compiler: Attach aliased Function to alias objects

9ecd075

FabioLuporini added 22 commits March 29, 2023 20:13

compiler: Fix is_on_device to work with aliases

658d9e6

compiler: Tweak _mark_overlappable pass

3c5135b

compiler: Enhance and polish reuse_efuncs

7da0ca9

compiler: Lift drop-halospot logic

a88ad2b

mpi: Make custom topos produce smaller HaloSchemes

119d9e7

compiler: Take down apparently useless uniquify_input

44d5f36

mpi: Rework CustomTopology logic

01a6dfe

compiler: Fix abstract_object for BlockDimensions

831446e

mpi: Improve custom topology effect

85b2b21

tests: Remove MPI-dual from OSS testing

54b50be

mpi: Tweak HaloScheme construction

623b77b

compiler: Postpone Communications pass

3529a06

bench: Improve and tweak profiling

60006bc

mpi: Rearrange dual mode codegen

3fb45ad

mpi: Identify Remainder callables

b25b6e0

compiler: Tweak IET reconstruction engine

c284283

compiler: Tweak (partial rollback) is_on_device

4848928

compiler: Improve data dep tracking across fences

ad7ccd2

misc: Add docstrings

5864ea6

tests: Reinstante test_cache_blocking_imperfect_nest

62e3ccd

compiler: Add more Properties handles

1d4b4aa

compiler: Improve par-tile selection for small ispaces

edb859d

FabioLuporini force-pushed the mpi-revamp branch from 6a40d52 to edb859d Compare March 29, 2023 20:14

mloubout approved these changes Apr 3, 2023

View reviewed changes

FabioLuporini added 2 commits April 5, 2023 10:55

mpi: Identify gather/scatter Calls

e09368b

bench: Emit GPts/s w/o setup time

536f560

FabioLuporini merged commit c506dca into master Apr 5, 2023
34 checks passed

FabioLuporini deleted the mpi-revamp branch April 5, 2023 12:35

		@@ -1,75 +1,61 @@
		from collections import defaultdict
		from itertools import groupby

compiler: Revamp compilation of halo exchanges #2089

compiler: Revamp compilation of halo exchanges #2089

Conversation

FabioLuporini commented Mar 16, 2023

codecov bot commented Mar 16, 2023 • edited

Codecov Report

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

FabioLuporini Mar 28, 2023 • edited

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

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 Mar 28, 2023

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2023 •

edited

FabioLuporini Mar 28, 2023 •

edited