compiler: Revamp data streaming #1702

FabioLuporini · 2021-06-07T12:40:55Z

This PR:

Fixes a host-side memory leak when streaming data from the host to device. This was due to the pthreads allocating so-called pinned memory under-the-hood to perform the transfers; the issue was that this memory never got freed. Now such pinned memory regions are created by the parent thread, through the use of buffers (hence the significant changes to the buffering pass) and by requesting the OpenACC runtime to always allocate pinned memory (option gpu=pinned added to NvidiaCompiler class)
Enables data streaming on AMD cards via OpenMP offloading (reusing part of the technology implemented for the bug fix above!)
Reworks and improves various components revolving around the entire data streaming process
Makes data streaming , and in particular host-to-device prefetching, more efficient
Adds quite a few new data streaming tests

codecov · 2021-06-07T12:44:27Z

Codecov Report

Merging #1702 (e21744f) into master (7f564c1) will decrease coverage by 0.56%.
The diff coverage is 75.12%.

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   89.10%   88.54%   -0.57%     
==========================================
  Files         207      208       +1     
  Lines       32759    33440     +681     
  Branches     4245     4352     +107     
==========================================
+ Hits        29190    29608     +418     
- Misses       3085     3334     +249     
- Partials      484      498      +14

Impacted Files	Coverage Δ
devito/arch/compiler.py	`52.39% <0.00%> (-2.24%)`	⬇️
devito/passes/equations/__init__.py	`100.00% <ø> (ø)`
devito/passes/iet/languages/openmp.py	`100.00% <ø> (ø)`
tests/test_adjoint.py	`100.00% <ø> (ø)`
tests/test_gpu_common.py	`1.70% <0.00%> (-0.20%)`	⬇️
devito/symbolics/queries.py	`39.72% <6.12%> (-8.97%)`	⬇️
devito/ir/support/vector.py	`88.23% <9.09%> (-5.58%)`	⬇️
devito/passes/clusters/asynchrony.py	`34.71% <38.56%> (-13.52%)`	⬇️
devito/passes/iet/orchestration.py	`60.89% <55.20%> (-4.71%)`	⬇️
devito/ir/iet/efunc.py	`90.00% <87.50%> (-0.48%)`	⬇️
... and 36 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 7f564c1...e21744f. Read the comment docs.

georgebisbas · 2021-06-15T08:21:32Z

devito/arch/compiler.py

@@ -460,7 +460,7 @@ def __init__(self, *args, **kwargs):
        self.cflags.remove('-std=c99')
        self.cflags.remove('-O3')
        self.cflags.remove('-Wall')
-        self.cflags += ['-std=c++11', '-acc:gpu', '-mp']
+        self.cflags += ['-std=c++11', '-acc:gpu', '-gpu=pinned', '-mp']


georgebisbas · 2021-06-15T08:28:29Z

devito/ir/iet/utils.py

    """
-    # TODO: this is currently very rudimentary
    required = derive_parameters(iet)


required assigned twice?

georgebisbas · 2021-06-15T08:33:50Z

devito/passes/clusters/asynchrony.py

-        # Attach SyncOps to Clusters
+        actions = defaultdict(Actions)
+
+        # Case 1


Any docs to provide more details for each case?

georgebisbas · 2021-06-15T08:48:06Z

devito/passes/clusters/buffering.py

+                else:
+                    size = async_degree
+
+            # Replace `d` with a suitable CustomDimension `bd`


bd-db mismatch ?

georgebisbas · 2021-06-15T08:50:27Z

devito/passes/clusters/buffering.py

+        `time` Dimension, we could have `{time: (time_m + db0) % 2, (time_m + db0)}`;
+        likewise, for backwards, `{time: (time_M - 2 + db0) % 4, time_M - 2 + db0}`.
+        """
+        mapper = {}


bd bm mismatch

yeah, you're very right, but the gencode name convention and the variable name convention in the python code is sometimes different. I'll see what needs to be done to fix this

mloubout

Except the Precedence thing I don't think makes sense, no other comments loks fine to me.

mloubout · 2021-06-15T20:13:13Z

devito/passes/iet/parpragma.py

+            else:
+                nofs, size = i
+                if isinstance(nofs, Mod):
+                    # NOTE: work around alleged SymPy bug which doesn't allow


As said on slack, it's not a sympy bug, it's a choice of devito to go with % while sympy uses fmod which is a function call and therefore always has precedence over Mul.
I think it makes much more sense to fix print_Mod rather than adding this odd extra type.

It's actually very easy to fix if you use ourMod instead of the sympy one and add

@property def precedence(self): return 49 # precedence(Mul)-1

since the first thing sympy checks is if it's defined before going to the defaults
https://github.com/sympy/sympy/blob/126f80578140e752ad5135aac77b8ff887eede3e/sympy/printing/precedence.py#L123

right. However, as explained on Slack, I forgot that this piece of code is actually dead, so I just dropped it

FabioLuporini added bug-performance-gpu GPU labels Jun 7, 2021

FabioLuporini requested a review from mloubout June 7, 2021 13:02

FabioLuporini force-pushed the revamp-streaming-final branch 3 times, most recently from a606c53 to b5669de Compare June 9, 2021 07:37

FabioLuporini mentioned this pull request Jun 15, 2021

examples: Switch to adjoint time derivative #1706

Merged

georgebisbas reviewed Jun 15, 2021

View reviewed changes

mloubout reviewed Jun 15, 2021

View reviewed changes

FabioLuporini and others added 20 commits June 16, 2021 15:55

tests: Pull buffering tests out of test_lower_exprs

ff51a3e

compiler: Tweak IterationInterval

52b6cce

compiler: Lower buffering from equations to clusters level

3bd4426

compiler: Add buffering of read-only Functions

bbae686

compiler: Strenghten buffering

a95476e

compiler: Enhance buffering to support unstructed read accesses

badb0a0

compiler: Enhance q_positive/q_negative

3376980

compiler: Use q_positive/q_negative for smart Vector comparison

9128992

compiler: Add support for buffering with ConditionalDimension

4ee2b9c

compiler: Patch buffering for shifted indexing

9277d58

compiler: Special-case 1-size buffering (generating neater code)

51f4d88

compiler: Patch buffering to work with arbitrary time_m

908a820

compiler: Use space=default with readonly buffers

468eb2b

compiler: Reuse buffer dimensions if possible

74b940f

compiler: Patch ScheduleTree construction

92e131d

compiler: Patch Call.functions

9a232ba

compiler: Add memcpy_to language constructs

fd9f60c

compiler: Patch DefFunction.free_symbols

2b2558f

compiler: Enable inline casting of pointers to C base types

f8c554c

compiler: Rework and enhance streaming+orchestration

43603ca

FabioLuporini added 19 commits June 16, 2021 15:55

tests: Add buffering+streaming tests

58ccb23

compiler: Patch ScheduleTree construction

6b0a3b2

compiler: Patch buffering to honor operands ordering

10aa9eb

compiler: Patch buffering update

34bed11

compiler: Fixup buffer initialization

c433cdf

compiler: Fix declarations within Conditionals

e96fb31

compiler: Rework and enhance streaming

55fc571

tests: Add test_streaming_complete

1a7e210

tests: Add test_everything stressing the buffering pass

9570988

compiler: Add Precedence to work around sympy deficiencies

015bf12

compiler: Patch memcpy with modulo-indexing

df48c40

mpi: Msg.function -> Msg.target, to avoid name clash

0ef7e56

arch: Use pinned memory systematically with Nvidia SDK

d49bade

compiler: Rework streaming to avoid host-side mem leaking

6141402

compiler: Deprecate cire-maxalias option (currently unused)

940ebde

tests: Enable streaming with openmp offloading

c631052

ci: Enable wampa to test openmp-offloading on AMD cards

95baf54

ci: Disable aomp+mpi tests as currently unsupported

034a74d

compiler: Drop forgotten dead code

e21744f

FabioLuporini force-pushed the revamp-streaming-final branch from 18765ab to e21744f Compare June 16, 2021 13:56

mloubout approved these changes Jun 16, 2021

View reviewed changes

FabioLuporini merged commit 87ee495 into master Jun 17, 2021

FabioLuporini deleted the revamp-streaming-final branch June 17, 2021 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Revamp data streaming #1702

compiler: Revamp data streaming #1702

FabioLuporini commented Jun 7, 2021 •

edited

Loading

codecov bot commented Jun 7, 2021 •

edited

Loading

georgebisbas Jun 15, 2021

georgebisbas Jun 15, 2021

georgebisbas Jun 15, 2021

georgebisbas Jun 15, 2021

georgebisbas Jun 15, 2021

FabioLuporini Jun 16, 2021

mloubout left a comment

mloubout Jun 15, 2021

mloubout Jun 16, 2021

FabioLuporini Jun 16, 2021

compiler: Revamp data streaming #1702

compiler: Revamp data streaming #1702

Conversation

FabioLuporini commented Jun 7, 2021 • edited Loading

codecov bot commented Jun 7, 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

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

FabioLuporini commented Jun 7, 2021 •

edited

Loading

codecov bot commented Jun 7, 2021 •

edited

Loading