compiler: Change tile use in DeviceAcczier to allow multiple tile sizes #2095

gpc1064 · 2023-03-29T14:22:47Z

A proposal to allow the use of multiple tile sizes in the same operator, through the part-tile flag.
Passing multiple tuples to the part-tile flag is already supported, but only the first tuple is applied in the _make_partree method of DeviceAcczier class.

The changes consist of supplying the nesting index in the make_parallel method of PragmaShmTransformer to the _make_partree method and using the tile size with the respective index.

Cases with different tile and nesting sizes are handled. Given n tile sizes for an operator with m nests:

If n = m: The i-th tile size is assigned to the i-th nesting.
If n > m: The first m tile sizes are assigned to the m nests (i-th tile size -> i-th nesting), and the remaining n-m is not used.
If n < m: The n tile sizes are assigned to the first n nestings, and the remaining m-n nestings replicate the last tile size.

georgebisbas

Thanks for this! As a first thing to improve, before the rest, could you add tests to this PR?

FabioLuporini

This PR needs to be equipped with tests, somewhere here: https://github.com/devitocodes/devito/blob/master/tests/test_dle.py#L265

and at least one in test_gpu_openacc.py as well

I've also left a batch of small comments

FabioLuporini · 2023-03-29T14:28:55Z

devito/passes/iet/languages/openacc.py

@@ -183,7 +183,10 @@ def _make_partree(self, candidates, nthreads=None):
            # TODO: still unable to exploit multiple par-tiles (one per nest)


You may now drop this comment

FabioLuporini · 2023-03-29T14:29:41Z

devito/passes/iet/languages/openacc.py

@@ -183,7 +183,10 @@ def _make_partree(self, candidates, nthreads=None):
            # TODO: still unable to exploit multiple par-tiles (one per nest)
            # This will require unconditionally applying blocking, and then infer
            # the tile clause shape from the BlockDimensions' step
-            tile = self.par_tile[0]
+            par_tile_length = len(self.par_tile)


how about simply n = ... ?

FabioLuporini · 2023-03-29T14:30:31Z

devito/passes/iet/languages/openacc.py

@@ -183,7 +183,10 @@ def _make_partree(self, candidates, nthreads=None):
            # TODO: still unable to exploit multiple par-tiles (one per nest)
            # This will require unconditionally applying blocking, and then infer
            # the tile clause shape from the BlockDimensions' step
-            tile = self.par_tile[0]
+            par_tile_length = len(self.par_tile)
+            idx = index if index < par_tile_length else par_tile_length - 1


we prefer explicit if-then-else:

if ...: idx = ... else: idx = ...

and in practice this is just

index = min(index, par_tile_length-1)

Which one is more suitable?

tile = self.par_tile[min(index, len(self.par_tile) - 1)]

idx = min(index, len(self.par_tile) - 1) tile = self.par_tile[idx]

I'd say the latter for homogeneity with the rest of the codebase.

FabioLuporini · 2023-03-29T14:31:02Z

devito/passes/iet/parpragma.py

            # Get the parallelizable Iterations in `tree`
            candidates = filter_iterations(tree, key=self.key)
            if not candidates:
                continue

            # Outer parallelism
-            root, partree = self._make_partree(candidates)
+            root, partree = self._make_partree(candidates, None, i)


instead of passing None:

self._make_partree(candidates, index=i)

…r in PragmaShmTransformer

…ng in test_gpu_openacc.py

gpc1064 · 2023-03-31T18:30:59Z

Thanks for this! As a first thing to improve, before the rest, could you add tests to this PR?

This PR needs to be equipped with tests, somewhere here: https://github.com/devitocodes/devito/blob/master/tests/test_dle.py#L265

and at least one in test_gpu_openacc.py as well

I've also left a batch of small comments

Added test_openacc_structure in test_dle.py::TestBlockingParTile to ensure correct blocking while used with OpenACC's tile.

Added test_openacc_multi_tile_structure in test_dle.py::TestBlockingParTile to ensure correct blocking and applying multiple tile sizes with OpenACC.

Added test_multiple_tile_sizes in test_gpu_openacc.py::TestCodeGeneration ensuring that multiple tile sizes are applied correctly across nests.

gpc1064 · 2023-03-31T18:34:49Z

Requested code changes also committed

FabioLuporini · 2023-04-01T12:07:30Z

tests/test_dle.py

+        (((32, 4, 4), 1, 'tag0'), ((4, 4, 32), (4, 4, 32))),
+        ((((32, 4, 8), 1, 'tag0'), ((32, 8, 4), 2)), ((8, 4, 32), (4, 8, 32))),
+    ])
+    def test_openacc_structure(self, par_tile, expected):


Sorry, I made a mistake; clearly these tests belong to test_gpu_openacc, not to test_dle which constitutes a platform-independent set of tests

(By moving them there you'll have to rename them as well -- dropping "openacc" for example)

FabioLuporini · 2023-04-01T12:09:13Z

tests/test_dle.py

+            assert all(i.step == j for i, j in zip(iters, v))
+
+
+    def test_openacc_multi_tile_structure(self):


how is this test conceptually different than the one above?

Indeed the first one was kind of unnecessary. I've dropped it and kept the second one, which is more relevant

FabioLuporini

OK I think this is now good to go. Enabling tests...

gpc1064 · 2023-04-20T14:40:18Z

Updated parameters receiving in _make_partree to include index. Local tests ran smoothly :)

codecov · 2023-04-27T09:35:02Z

Codecov Report

Merging #2095 (c294588) into master (d0e705d) will decrease coverage by 0.25%.
The diff coverage is 17.50%.

@@            Coverage Diff             @@
##           master    #2095      +/-   ##
==========================================
- Coverage   87.80%   87.56%   -0.25%     
==========================================
  Files         221      221              
  Lines       38665    39042     +377     
  Branches     5000     5071      +71     
==========================================
+ Hits        33951    34188     +237     
- Misses       4164     4295     +131     
- Partials      550      559       +9

Impacted Files	Coverage Δ
tests/test_gpu_openacc.py	`5.69% <6.06%> (-1.40%)`	⬇️
devito/passes/iet/languages/openacc.py	`65.00% <33.33%> (-0.55%)`	⬇️
devito/passes/iet/parpragma.py	`87.61% <100.00%> (+0.95%)`	⬆️

... and 34 files with indirect coverage changes

compiler: Change tile use in DeviceAcczier to allow multiple tile sizes

d1bb6c0

mloubout added GPU compiler labels Mar 29, 2023

georgebisbas requested changes Mar 29, 2023

View reviewed changes

FabioLuporini requested changes Mar 29, 2023

View reviewed changes

Gabriel Pinheiro added 2 commits March 31, 2023 14:51

compiler: refactor idx definition in DeviceAcczier and index paramete…

0c522f0

…r in PragmaShmTransformer

tests: add blocking and tiling tests in test_dle.py and multiple tili…

458f76c

…ng in test_gpu_openacc.py

FabioLuporini requested changes Apr 1, 2023

View reviewed changes

tests: refactor gpu openacc tests

7688522

FabioLuporini approved these changes Apr 5, 2023

View reviewed changes

compiler: fix _make_partree parameter reception

28fb81d

misc: fix indentation issues with pep8

c294588

FabioLuporini merged commit 12544a8 into devitocodes:master May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Change tile use in DeviceAcczier to allow multiple tile sizes #2095

compiler: Change tile use in DeviceAcczier to allow multiple tile sizes #2095

gpc1064 commented Mar 29, 2023

georgebisbas left a comment

FabioLuporini left a comment

FabioLuporini Mar 29, 2023

FabioLuporini Mar 29, 2023

FabioLuporini Mar 29, 2023

FabioLuporini Mar 29, 2023

gpc1064 Mar 29, 2023

FabioLuporini Mar 30, 2023

FabioLuporini Mar 29, 2023

gpc1064 commented Mar 31, 2023

gpc1064 commented Mar 31, 2023

FabioLuporini Apr 1, 2023

FabioLuporini Apr 1, 2023

FabioLuporini Apr 1, 2023

gpc1064 Apr 3, 2023

FabioLuporini left a comment

gpc1064 commented Apr 20, 2023

codecov bot commented Apr 27, 2023 •

edited

Loading

		@@ -183,7 +183,10 @@ def _make_partree(self, candidates, nthreads=None):
		# TODO: still unable to exploit multiple par-tiles (one per nest)

		assert all(i.step == j for i, j in zip(iters, v))


		def test_openacc_multi_tile_structure(self):

compiler: Change tile use in DeviceAcczier to allow multiple tile sizes #2095

compiler: Change tile use in DeviceAcczier to allow multiple tile sizes #2095

Conversation

gpc1064 commented Mar 29, 2023

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

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

gpc1064 commented Mar 31, 2023

gpc1064 commented Mar 31, 2023

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

gpc1064 commented Apr 20, 2023

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

codecov bot commented Apr 27, 2023 •

edited

Loading