Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create separate ModuleWrapTransformation from DependencyTransformation #197

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

reuterbal
Copy link
Collaborator

@reuterbal reuterbal commented Dec 8, 2023

The DependencyTransformation is responsible for an increasing number of workarounds and special casing in the scheduler traversals. Part of this has been identified to be related to the fact that two concepts are mixed together here:

  1. Renaming of scopes (modules/subroutines get new names, which requires renaming on the caller side)
  2. Changing scopes (subroutines get embedded into modules)

This PR splits the two into separate transformation passes but tries to be minimally invasive otherwise. There may undoubtedly be capacity in rewriting some of the transformation logic itself but that can now be a separate change.

The original strict mode behaviour is retained by plainly calling the DependencyTransformation, the module mode is recovered by applying the new ModuleWrapTransformation before the DependencyTransformation.

A caveat is the fact that the current scheduler items loose the ability to identify inline calls as targets after the module wrapping. This is because these links are only identified for functions that are declared via explicit interfaces, and after the module wrap these are regular fortran imports via use. Previously this wasn't a problem because the wrapping and renaming happened in the same step, and afterwards in the transformation pipeline this relation was no longer required.

To retain this piece of information across the now two independent transformation passes I converted the targets property to a cached property. This seems to be ok for all tests and regression tests but needs careful verification on ecphys! It's important to note that this is only a temporary measure because the scheduler rewrite will introduce inline call dependencies independently from an interface declaration.

Copy link

github-actions bot commented Dec 8, 2023

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

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3a06456) 92.21% compared to head (e252bfe) 92.22%.

Files Patch % Lines
loki/transform/dependency_transform.py 96.61% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
+ Coverage   92.21%   92.22%   +0.01%     
==========================================
  Files          94       94              
  Lines       16945    16949       +4     
==========================================
+ Hits        15625    15632       +7     
+ Misses       1320     1317       -3     
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.20% <96.63%> (+0.02%) ⬆️
transformations 91.45% <ø> (ø)

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.

@reuterbal reuterbal force-pushed the nabr-split-dependency-transform branch from b3bfae6 to e252bfe Compare December 8, 2023 11:44
@reuterbal reuterbal requested a review from mlange05 December 8, 2023 13:24
@reuterbal reuterbal marked this pull request as ready for review December 8, 2023 13:24
Copy link
Collaborator

@mlange05 mlange05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and works with current EC-physics regression. I think the cached target property was the hack trick I was missing when I tried this recently. As this will be removed soon, I'm happy for this to go ahead as is. GTG from me.

@reuterbal reuterbal merged commit 37d0033 into main Dec 12, 2023
11 checks passed
@reuterbal reuterbal deleted the nabr-split-dependency-transform branch December 12, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants