compiler: Introduce gpu-create parameter for buffers initialised on device #2107

ccuetom · 2023-04-14T15:35:11Z

Introduces a parameter devicecreate that defines a list of buffers to be created on the device and initialised to zero instead of copied from host to device.

For a buffer u to be created on the device, an operator can be created using op(..., devicecreate=u, ...).

For a series of buffers u and v the operator can be created using op(..., devicecreate=(u, v), ...).

The PR also marks recursive compilations with the flag is_rcompile and changes the iet_pass decorator to accept an optional parameter iet_pass(skipif_rcompile=True) that skips passes if part of a recursive compilation. This prevents infinite recursions when calling rcompile from an iet_pass.

FabioLuporini

Awesome !

I just left a few minor comments

FabioLuporini · 2023-04-17T07:44:55Z

devito/core/gpu.py

@@ -73,6 +73,7 @@ def _normalize_kwargs(cls, **kwargs):
        o['par-nested'] = np.inf  # Never use nested parallelism
        o['par-disabled'] = oo.pop('par-disabled', True)  # No host parallelism by default
        o['gpu-fit'] = as_tuple(oo.pop('gpu-fit', cls._normalize_gpu_fit(**kwargs)))
+        o['devicecreate'] = as_tuple(oo.pop('devicecreate', ()))


OK, in retrospect, maybe gpu-create, is a better candidate. Or perhaps gpu-zero?

I've changed it to gpu-create, I think gpu-zero gives the impression that it is only zeroed within the device and gives no information about where the buffer is generated

FabioLuporini · 2023-04-17T07:49:35Z

devito/operator/operator.py

@@ -1086,10 +1090,14 @@ def parse_kwargs(**kwargs):
    else:
        openmp = kwopenmp

+    # `opt`, devicecreate
+    devicecreate = kwargs.get('devicecreate', options.get('devicecreate', ()))


I don't know where to write this comment, so I'll write it here.

Currently the API is:

Operator(... devicecreate=... )

But devicecreate is a so called opt-option (optimization option), hence it should rather be passed as such:

Operator(..., opt=('advanced', {'devicecreate': ...} # or rather gpu-zero, see above

So you can drop this change here, and rather add it here
https://github.com/devitocodes/devito/blob/master/devito/core/cpu.py#L72
and here
https://github.com/devitocodes/devito/blob/master/devito/core/gpu.py#L75
(in fact, you've already started editing at the second link...)

EDIT: uops, sorry, just saw that you indeed have performed edits at both links already 👍

FabioLuporini · 2023-04-17T07:50:19Z

devito/passes/__init__.py

@@ -39,6 +39,36 @@ def is_on_device(obj, gpu_fit):
    return all(f in gpu_fit for f in fsave)


+def is_device_created(obj, devicecreate):


is_gpu_zero?

Worth mentioning in the doc that the Functions will be zero-initialized

FabioLuporini · 2023-04-17T07:51:27Z

devito/passes/iet/definitions.py

@@ -393,7 +393,7 @@ def place_casts(self, iet, **kwargs):

        return iet, {}

-    def process(self, graph):
+    def process(self, graph, **kwargs):


you're stashing devicecreate (gpu-zero or whatever) in __init__ -- correctly! Hence you don't need to add **kwargs, neither here nor at the caller sites

These **kwargs are needed to have is_rcompile reach the iet_pass wrapper

FabioLuporini · 2023-04-17T07:53:27Z

devito/passes/iet/definitions.py

+            mmap = (mmap, init)
+        else:
+            mmap = self.lang._map_to(obj)
+            efunc = ()

        if read_only is False:


does the case ìs_device_created+ read_only make sense? ie, both ifs are true

probably not now that I think of it

FabioLuporini · 2023-04-17T07:57:57Z

devito/passes/iet/definitions.py

+        if is_device_created(obj, self.devicecreate):
+            mmap = self.lang._map_alloc(obj)
+
+            cdims = []


Could we move this entire segment of code -- from cdims = [] to the end of the for loop (line 465) to a separate utility function? Something at the bottom of this file would do. Something like make_zero_init or whatever. I could then use it from other places as well, thus avoiding redundant code.

FabioLuporini · 2023-04-17T08:01:45Z

tests/test_gpu_common.py

@@ -1128,6 +1128,44 @@ def test_xcor_from_saved(self, opt, opt_options, gpu_fit):

        assert np.all(g.data == 30)

+    def test_devicecreate_forward(self):


on second thought, I think we need to check codegen. Something very minimal -- eg to make sure that there's indeed a create(...) and not a mapto().

if openacc: assert 'create(u)' in str(...) elif openmp assert '...'

(it's target-language specific; you can pull the target language from configuration)

FabioLuporini · 2023-04-17T08:03:04Z

devito/core/cpu.py

@@ -179,7 +180,7 @@ def _specialize_iet(cls, graph, **kwargs):
        hoist_prodders(graph)

        # Symbol definitions
-        cls._Target.DataManager(**kwargs).process(graph)
+        cls._Target.DataManager(**kwargs).process(graph, **kwargs)


see comments below -- but basically you don't need to pass kwargs here

This is still needed to pass is_rcompile to the iet_pass wrapper

mloubout

Some minor comments, looks nice

mloubout · 2023-04-17T14:27:59Z

devito/passes/__init__.py

+            f = i
+        fcreate.append(f)
+
+    return all(f in gpu_create for f in fcreate)


nitpicking in case list is "long". You don't actually need other fcreate you just need to return false at the first i not in gpu_create and return true at the end

that's true, hadn't thought of it

devito/passes/iet/definitions.py

mloubout · 2023-04-17T14:32:55Z

tests/test_gpu_common.py

+
+        assert np.all(u.data[0] == 36)
+        assert np.all(u.data[1] == 35)
+


Should place-transfers be tested out as well separatrly

I can do that. Should I add the test to test_gpu_common.py::TestStreaming?

… loop

FabioLuporini

Left tiny comments, but imho we can land this

FabioLuporini · 2023-04-18T07:40:11Z

devito/passes/__init__.py

+    gpu_create : list of Function
+        The Function's which are expected to be created in device memory. This
+        information is given directly by the user through the compiler option
+        `devicecreate` and is propagated down here through the various stages of lowering.


FabioLuporini · 2023-04-18T07:40:59Z

devito/passes/__init__.py

@@ -39,6 +39,37 @@ def is_on_device(obj, gpu_fit):
    return all(f in gpu_fit for f in fsave)


+def is_gpu_create(obj, gpu_create):
+    """
+    True if the given object is created and not copied in the device memory,


:%s/object is/objects are

?

devito/passes/iet/definitions.py

FabioLuporini

LGTM, running tests

codecov · 2023-04-19T07:29:13Z

Codecov Report

Merging #2107 (89e33fc) into master (468e493) will decrease coverage by 0.13%.
The diff coverage is 23.76%.

@@            Coverage Diff             @@
##           master    #2107      +/-   ##
==========================================
- Coverage   87.73%   87.60%   -0.13%     
==========================================
  Files         221      221              
  Lines       38961    39100     +139     
  Branches     5064     5080      +16     
==========================================
+ Hits        34181    34253      +72     
- Misses       4213     4280      +67     
  Partials      567      567

Impacted Files	Coverage Δ
tests/test_gpu_common.py	`1.32% <0.00%> (-0.08%)`	⬇️
devito/passes/iet/definitions.py	`82.31% <31.25%> (-6.03%)`	⬇️
devito/passes/__init__.py	`66.66% <46.15%> (-11.60%)`	⬇️
devito/core/gpu.py	`73.75% <71.42%> (-0.08%)`	⬇️
devito/core/cpu.py	`100.00% <100.00%> (ø)`
devito/operator/operator.py	`89.15% <100.00%> (+0.02%)`	⬆️

... and 9 files with indirect coverage changes

FabioLuporini

GTG. Next in the merge pipeline... Thanks!

ccuetom added 3 commits April 14, 2023 16:23

compiler: Introduce devicecreate parameter

dfdc21a

compiler: Add zero initilisation for devicecreate parameters

0016521

compiler: Add devicecreate tests to test_gpu_common

b53d1d7

georgebisbas added the compiler label Apr 14, 2023

FabioLuporini requested changes Apr 17, 2023

View reviewed changes

ccuetom added 3 commits April 17, 2023 10:24

compiler: Change devicecreate to gpu-create and test its codegen

b1b6302

compiler: Substitute rcompile flag for place-transfers operator option

aebf757

compiler: Fix flake8 E501

4db57d5

mloubout reviewed Apr 17, 2023

View reviewed changes

ccuetom added 2 commits April 17, 2023 15:55

compiler: Make is_gpu_create return early instead of completing whole…

6f41377

… loop

compiler: Add test for place-transfers flag

e6fedd1

FabioLuporini changed the title ~~compiler: Introduce devicecreate parameter for buffers initialised on device~~ compiler: Introduce gpu-create parameter for buffers initialised on device Apr 18, 2023

FabioLuporini approved these changes Apr 18, 2023

View reviewed changes

compiler: Fix docstring for is_gpu_create

941d59d

FabioLuporini approved these changes Apr 19, 2023

View reviewed changes

ccuetom added 2 commits April 19, 2023 09:32

compiler: Fix flake8 F841 and E501

00c350f

compiler: Prevent gpu-create use with omp

89e33fc

FabioLuporini approved these changes May 3, 2023

View reviewed changes

FabioLuporini merged commit b858fb1 into devitocodes:master May 3, 2023

ccuetom deleted the openacc-create branch May 19, 2023 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Introduce gpu-create parameter for buffers initialised on device #2107

compiler: Introduce gpu-create parameter for buffers initialised on device #2107

ccuetom commented Apr 14, 2023

FabioLuporini left a comment

FabioLuporini Apr 17, 2023

ccuetom Apr 17, 2023

FabioLuporini Apr 18, 2023

FabioLuporini Apr 17, 2023

FabioLuporini Apr 17, 2023

FabioLuporini Apr 17, 2023

ccuetom Apr 17, 2023

FabioLuporini Apr 17, 2023

ccuetom Apr 17, 2023

FabioLuporini Apr 17, 2023

FabioLuporini Apr 17, 2023

FabioLuporini Apr 17, 2023

ccuetom Apr 17, 2023

mloubout left a comment

mloubout Apr 17, 2023

ccuetom Apr 17, 2023

mloubout Apr 17, 2023

ccuetom Apr 17, 2023

FabioLuporini left a comment

FabioLuporini Apr 18, 2023

FabioLuporini Apr 18, 2023

FabioLuporini left a comment

codecov bot commented Apr 19, 2023 •

edited

Loading

FabioLuporini left a comment

		@@ -39,6 +39,36 @@ def is_on_device(obj, gpu_fit):
		return all(f in gpu_fit for f in fsave)


		def is_device_created(obj, devicecreate):

		@@ -1128,6 +1128,44 @@ def test_xcor_from_saved(self, opt, opt_options, gpu_fit):

		assert np.all(g.data == 30)

		def test_devicecreate_forward(self):


		assert np.all(u.data[0] == 36)
		assert np.all(u.data[1] == 35)

compiler: Introduce gpu-create parameter for buffers initialised on device #2107

compiler: Introduce gpu-create parameter for buffers initialised on device #2107

Conversation

ccuetom commented Apr 14, 2023

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

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

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

FabioLuporini left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2023 • edited Loading

Codecov Report

FabioLuporini left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2023 •

edited

Loading