Transformations: "Parallel" sub-module with driver-level parallelilsation utilities #415

mlange05 · 2024-10-25T08:04:11Z

Note: This sits on top of #414 , as it requires the new Dimension overloading mechanism for block-loop dimensions.

This change brings a set of utilities that form the basis of "driver"-level control flow manipulation for IFS-style block loops, OpenMP parallel regions and FIELD API boilerplate code. They are loose packaged in a new sub-module loki.transformations.parallel and are intended to be extended at a later stage. The come in remove / add pairs, and currently only pertain to host-side driver loops with FIELD-API view updates.

I've also piggy-backed two small IR-level fixes (logging string and a weird corner case for pragma region detection), the latter of which was needed for some of the testing of the new functionalities.

In a little more detail, the new sub-package provides:

Utilities to remove !$omp parallel sections and optionally inject !$loki parallel regions in their place.
A utility to re-inject !$omp parallel sections for !$loki parallel markers, where "private" and "firstprivate" clauses are derived using dataflow analysis
Utilities to remove and create explicit copies for derived types that would trigger a known "firstprivate" bug. This common workarounds is inserted for explicitly named variables and is thus user-driven.
Utilities to remove and create the common FIELD API object%update_view(ibl) for derived-type objects that have specifically named types. This can also insert the slightly deviating dimension%update(ibl, icst, icend) updates.

The transformations that require detection of IFS-style "driver" block loops depend on a specific overloading of the primary/secondary lower/upper/step properties of the re-written Dimension object - a very specific convention that I'm open to discussing further (still seems hacky!). 😞

github-actions · 2024-10-25T08:06:51Z

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/415/index.html

wertysas · 2024-11-11T09:40:23Z

loki/transformations/parallel/field_api.py

+]
+
+
+def remove_field_api_view_updates(routine, field_group_types, dim_object=None):


This function is case-sensitive, but it may be good to make it case-insensitive.

Nice catch! The transformation is actual case-insensitive, but the warning checks are not. I've added come camel-case testing and checking of the generated warnings now.

codecov · 2024-11-11T14:39:53Z

Codecov Report

Attention: Patch coverage is 99.06542% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.23%. Comparing base (358fb60) to head (2d361bc).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
loki/transformations/parallel/openmp_region.py	97.89%	2 Missing ⚠️
loki/ir/pragma_utils.py	0.00%	1 Missing ⚠️
loki/transformations/parallel/field_api.py	98.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   93.16%   93.23%   +0.07%     
==========================================
  Files         205      212       +7     
  Lines       39832    40259     +427     
==========================================
+ Hits        37109    37535     +426     
- Misses       2723     2724       +1

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`93.18% <99.06%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wertysas

I left some comments, mostly on typos in docstrings. It was useful for me to see some good examples of the visitor pattern in action in Loki! The caplog functionality of pytests seems very useful, I didn't know that it was this easy to check the warning messages.

loki/transformations/parallel/block_loop.py

loki/transformations/parallel/field_api.py

loki/transformations/parallel/block_loop.py

loki/transformations/parallel/openmp_region.py

loki/transformations/parallel/tests/test_field_api.py

awnawab

Many many thanks for these great utilities 🙏 The lack of driver layer transformations has led to a not insignificant amount of code duplication in certain places, which of course causes conflicts when merging science changes. So these will really help cleanup loki integration with the IFS 🥳

I've left a bunch of comments that I think should be addressed, mostly about extending test coverage and typos. There are also some more nitpicky ones which I've marked as optional.

Finally, these utilities in their current guise won't quite work out-of-the-box on ecWAM or the IFS dy-core, but I will address that in followup PRs.

awnawab · 2024-11-12T10:27:50Z

loki/transformations/parallel/block_loop.py

+    """
+    Remove any outer block :any:`Loop` from a given :any:`Subroutine.
+
+    The loops are identified accoerding to a given :any:`Dimension`


Typo: according

awnawab · 2024-11-13T09:06:45Z

loki/transformations/parallel/openmp_region.py

+
+        def visit_PragmaRegion(self, region, **kwargs):
+            """
+            Perform the fileterin and removeal of OpenMP pragma regions.


Typo: Filtering and removal

awnawab · 2024-11-13T09:09:07Z

loki/transformations/parallel/openmp_region.py

+                return region
+
+            if kwargs['active'] and region.pragma.keyword.lower() == 'omp':
+                # Remove other OpenMP pragam regions when in active mode


Typo: OpenMP pragma regions

awnawab · 2024-11-13T09:12:14Z

loki/transformations/parallel/openmp_region.py

+    Add the OpenMP directives for a parallel driver region with an
+    outer block loop.
+    """
+    block_dim_size = 'YDGEOMETRY%YRDIM%NGPBLKS'


This should probably be configurable, so perhaps a block_dim_size argument with a default value of YDGEOMETRY%YRDIM%NGPBLKS.

awnawab · 2024-11-13T09:15:35Z

loki/transformations/parallel/openmp_region.py

+
+                # Accumulate the set of locally used symbols and chase parents
+                symbols = tuple(region.uses_symbols | region.defines_symbols)
+                symbols = tuple(dict.fromkeys(flatten(


Optional: this could also be symbols = tuple(set(routine.variable_map[s.name_parts[0]] for s in symbols)), which is perhaps a little more intuitive.

Never mind, my suggestion doesn't make sense, I had originally misunderstood what this line does. Sorry for the noise 😅

awnawab · 2024-11-13T09:48:07Z

loki/transformations/parallel/openmp_region.py

+    with pragma_regions_attached(routine):
+        with dataflow_analysis_attached(routine):
+            for region in FindNodes(ir.PragmaRegion).visit(routine.body):
+                if not is_loki_pragma(region.pragma, starts_with='parallel'):


Optional: this bailout is untested.

awnawab · 2024-11-13T09:49:58Z

loki/transformations/parallel/openmp_region.py

+        def visit_PragmaRegion(self, region, **kwargs):  # pylint: disable=unused-argument
+            # Apply to pragma-marked "parallel" regions only
+            if not is_loki_pragma(region.pragma, starts_with='parallel'):
+                return region


Optional: this bailout is untested.

awnawab · 2024-11-13T09:50:55Z

loki/transformations/parallel/openmp_region.py

+            lhs = assign.lhs.name
+            if lhs in fprivate_map and assign.rhs == fprivate_map[lhs]:
+                return None
+            return assign


Could we please test that the assignments that aren't in the privatisation map remain unaffected?

awnawab · 2024-11-13T10:13:48Z

loki/transformations/parallel/openmp_region.py

+    """
+    block_dim_size = 'YDGEOMETRY%YRDIM%NGPBLKS'
+
+    global_variables = global_variables or {}


As I understand it, these are the variables that compilers struggle to firstprivatise, so perhaps it should be renamed to skip_firstprivatise_vars or something similar. global_variables should be reserved for module imports. And could the tests also be extended to cover this functionality?

Agreed that "global" is ambiguous in this context, but skip_firstprivate_vars is also to long and unclear (they can also not be "private"). How about shared_variables to stick with OpenMP semantics?

awnawab · 2024-11-13T10:17:09Z

loki/transformations/parallel/openmp_region.py

+                local_vars = tuple(v for v in local_vars if v.name not in global_variables)
+
+                # Make field group types firstprivate
+                firstprivates = tuple(dict.fromkeys(


[noaction] In principle it would be great to rely primarily on the dataflow to determine what should be privatised and firstprivatised with a few known overrides. Unfortunately this quickly becomes impractical due to the missing intents on the SELF dummy argument in the type-bound procedures, not to mention the need to first parse all of the corresponding type-definitions. So for now we can leave this as is, but it would be nice to generalise this in a future PR.

Agreed, but as I have a bunch of other modifications for this already lined up, I'd like to push this out to future PRs for now (when I have better real-life coverage for this).

mlange05 · 2024-11-14T09:28:12Z

Many thanks for the thorough review, @wertysas and @awnawab . I've pushed updates to the tests, comments and docstrings with small modifications to the methods themselves to address your comments. I've also renamed some of the "firstprivate-copies" routines to bring them in line with the naming convention for the other utilities. I've kept everything clean on top of the current branch to give you a clean diff - please let me know when if you're happy, so I can do another quick rebase over main.

wertysas · 2024-11-14T16:04:56Z

loki/transformations/parallel/openmp_region.py

@@ -53,7 +52,7 @@ class RemoveOpenMPRegionTransformer(Transformer):

        def visit_PragmaRegion(self, region, **kwargs):
            """
-            Perform the fileterin and removeal of OpenMP pragma regions.
+            Perform the filetering and removal of OpenMP pragma regions.


Suggested change

Perform the filetering and removal of OpenMP pragma regions.

Perform the filtering and removal of OpenMP pragma regions.

Yes, thanks, will mop this up in the rebase.

awnawab

Many thanks for addressing all the comments, this now looks good to go 👌

wertysas · 2024-11-14T16:28:09Z

loki/transformations/parallel/openmp_region.py

@@ -96,14 +95,26 @@ def visit_Pragma(self, pragma, **kwargs):
        routine.body = RemoveOpenMPRegionTransformer().visit(routine.body, active=False)


-def add_openmp_regions(routine, global_variables=None, field_group_types=None):
+def add_openmp_regions(
+        routine, dimension=None, shared_variables=None, field_group_types=None


Suggested change

routine, dimension=None, shared_variables=None, field_group_types=None

routine, dimension, shared_variables=None, field_group_types=None

I didn't realise this until the dimension was added to filter loops by loop variable, but attributes of the dimension parameter are accessed in this method, hence it shouldn't be set to None.

Nice, very good catch! Again, I'll mop this up in the rebase.

Optionally, this utility can also replace these outer pragmas with `!$loki parallal` pragma markers.

OMNI has trouble when combining Loop and region pragmas, since it swallows the empy lines we use to differentiate them.

This is to shorten the method names and bring them in line with the naming convention of the sub-package.

reuterbal

Thanks, this looks great!

Many thanks also to @awnawab and @wertysas for your diligent review and strict enforcement of testing - much appreciated!

mlange05 requested review from reuterbal and awnawab October 25, 2024 08:04

mlange05 force-pushed the naml-transformations-parallel branch from 8d95562 to dd545a0 Compare November 7, 2024 19:40

wertysas reviewed Nov 11, 2024

View reviewed changes

wertysas mentioned this pull request Nov 11, 2024

DataOffload: Add new FieldOffloadTransformation for FIELD API boilerplate injection #437

Merged

mlange05 force-pushed the naml-transformations-parallel branch from 57eab72 to c42095b Compare November 11, 2024 14:12

mlange05 marked this pull request as ready for review November 11, 2024 14:54

wertysas reviewed Nov 13, 2024

View reviewed changes

awnawab requested changes Nov 13, 2024

View reviewed changes

wertysas reviewed Nov 14, 2024

View reviewed changes

awnawab approved these changes Nov 14, 2024

View reviewed changes

wertysas reviewed Nov 14, 2024

View reviewed changes

mlange05 added 15 commits November 14, 2024 16:34

IR: Small fix for an erroneous warning message

153176a

Parallel: Add utility to remove existing OpenMP parallel region

3a5ae2f

Optionally, this utility can also replace these outer pragmas with `!$loki parallal` pragma markers.

Parallel: Add utility to insert host-side OpenMP thread pragmas

29dc638

Parallel: Ensure reproducible variable ordering in pragmas

8e3b8db

Parallel: Add utility to remove parallel block loops

474681a

Parallel: Add utility to insert block loops into routines

167a9ba

Parallel: Add utility to add/remove FIELD-API view updates

2fdb580

Parallel: Add utility to manage explicit firstprivate copies

efe4be8

Parallel: Add DEFAULT(SHARED) to OpenMP parallel clause creation

180fd83

Parallel: Disable OMNI for OpenMP region insertion test

b973bbb

OMNI has trouble when combining Loop and region pragmas, since it swallows the empy lines we use to differentiate them.

Parallel: Add camel-case testing and check generated warnings

db6df94

Parallel: Fixing typos, comments and imports

6e60ce1

Parallel: Better test coverage and case sensitivity for F-API removal

4a1fada

Parallel: Use Dimension and improve tests for add_openmp_region

c011e71

Parallel: Rename argument global_variables => shared_variables

c4f0bc5

mlange05 added 3 commits November 14, 2024 16:38

Parallel: Add more test coverage for corner cases

a3b85d2

Parallel: Rename to add/remove_firstprivate_copies

efea62f

This is to shorten the method names and bring them in line with the naming convention of the sub-package.

Parallel: Add missing recursion on remove_block_loops

2d361bc

mlange05 force-pushed the naml-transformations-parallel branch from c6c7b50 to 2d361bc Compare November 14, 2024 16:38

wertysas approved these changes Nov 14, 2024

View reviewed changes

mlange05 added the ready for merge This PR has been approved and is ready to be merged label Nov 14, 2024

reuterbal approved these changes Nov 15, 2024

View reviewed changes

mlange05 merged commit 00d4290 into main Nov 15, 2024
13 checks passed

mlange05 deleted the naml-transformations-parallel branch November 15, 2024 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformations: "Parallel" sub-module with driver-level parallelilsation utilities #415

Transformations: "Parallel" sub-module with driver-level parallelilsation utilities #415

mlange05 commented Oct 25, 2024

github-actions bot commented Oct 25, 2024

wertysas Nov 11, 2024

mlange05 Nov 11, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading

wertysas left a comment

awnawab left a comment

awnawab Nov 12, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

awnawab Nov 13, 2024

mlange05 Nov 13, 2024

awnawab Nov 13, 2024 •

edited

Loading

mlange05 Nov 14, 2024

mlange05 commented Nov 14, 2024

wertysas Nov 14, 2024

mlange05 Nov 14, 2024

awnawab left a comment

wertysas Nov 14, 2024

mlange05 Nov 14, 2024

reuterbal left a comment

		]


		def remove_field_api_view_updates(routine, field_group_types, dim_object=None):

	Perform the filetering and removal of OpenMP pragma regions.
	Perform the filtering and removal of OpenMP pragma regions.

	routine, dimension=None, shared_variables=None, field_group_types=None
	routine, dimension, shared_variables=None, field_group_types=None

Transformations: "Parallel" sub-module with driver-level parallelilsation utilities #415

Transformations: "Parallel" sub-module with driver-level parallelilsation utilities #415

Conversation

mlange05 commented Oct 25, 2024

github-actions bot commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

wertysas left a comment

Choose a reason for hiding this comment

awnawab 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

awnawab Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlange05 commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awnawab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 •

edited

Loading

awnawab Nov 13, 2024 •

edited

Loading