(Closes #3503) add LFRic meta-transformation for colouring and OpenMP - #3593
haanahfrost wants to merge 6 commits into
Conversation
haanahfrost
added
enhancement
LFRic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3593 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 401 402 +1
Lines 56540 56581 +41
=========================================
+ Hits 56540 56581 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LonelyCat124
left a comment
LonelyCat124
left a comment
There was a problem hiding this comment.
@haanahfrost Couple of change, most are just documentation or type hint related.
I've asked for one additional thing in the tests because I'm not entirely sure I know what will happen and I'd like it to be explicit in the tests as well.
|
|
||
| def validate(self, node: Routine, | ||
| reprod: Union[bool, None] = None, **kwargs): | ||
| # pylint: disable=arguments-renamed |
There was a problem hiding this comment.
can you add a TODO around this pylint as if options are removed it should no longer be necessary.
|
|
||
| :param node: the Routine node to transform | ||
| :param reprod: whether to use reproducible form of OpenMP reduction. | ||
| If none, the default value from the configuration is used. |
There was a problem hiding this comment.
This line of docstring should be indented.
| continuous function space. | ||
|
|
||
| :param node: the Routine whose loops are to be coloured. | ||
| :param colour_kwargs: keyword arguments for the LFRicColourTrans |
There was a problem hiding this comment.
If we're documenting this it needs to be type hinted - I think it should be dict[str, Any] but double check. Also for _parallelise_loops
There was a problem hiding this comment.
checked this with mypy. For **kwargs the annotation applies to each value rather than the dict itself so **colour_kwargs: Any reveals as dict[str, Any]
Annotating it dict[str, Any] gives dict[str, dict[str, Any]] and mypy rejects passing tiling=True.
Gone with Any for both _colour_loops and _parallelise_loops which also matches lfric_invoke_schedule.py
|
|
||
| :param node: the Routine whose loops are to be parallelised. | ||
| :param reprod: whether to use reproducible form of OpenMP reduction. | ||
| If none, the default value from the configuration is used. |
There was a problem hiding this comment.
Indent this line please.
| oregtrans = OMPParallelTrans() | ||
|
|
||
| # TODO #2668: LFRicOMPLoopTrans has not yet been migrated to kwargs. It | ||
| # it gives options inherrited from OMPLoopTrans but discards any |
There was a problem hiding this comment.
s/inherrited/inherited
| ''' | ||
| :param node: the Routine node to transform | ||
| :param reprod: whether to use reproducible form of OpenMP reduction. | ||
| If none, the default value from the configuration is used. |
| dist_mem=False) | ||
| sched = invoke.schedule | ||
| # w3 is discontinuous so no colouring happens. Parallelise the loop by | ||
| # hand first so that the transformation finds it already in a directive. |
There was a problem hiding this comment.
What happens if we have a continuous space and we already parallelise the loop? Can you test that behaviour as well?
There was a problem hiding this comment.
Tested this and it silently does nothing. Once OMPParallelTrans has been applied the loop is no longer a direct child of the Routine (children go from ['LFRicLoop'] to ['OMPParallelDirective']), so _colour_loops never sees it, and _parallelise_loops skips it because it has a Directive ancestor. This results in an uncoloured loop on a continuous space inside a parallel region, which is the race condition colouring is there to prevent.
Do you want validate to reject this? (raise if any loop with a Directive ancestor is on a continuous space)
Or should the test just document current behaviour?
2 participants
Adds
LFRicColourAndOMPTrans, a meta-transformation that colours the loopsin an LFRic
Routineand then applies OpenMP parallelism to them. Thisreplaces
colour_loopsandopenmp_parallelise_loopsfrom the Met Office'spsyclone_tools.py, so that the functionality lives in PSyclone and isproperly tested.
WIP: