SCC: Support for bounds aliases and derived type members as bounds #250

awnawab · 2024-03-18T09:28:48Z

This is the final PR needed to transform the SL. It consists of a series of fixes to the SCC transforms, the biggest of which is the ability to use aliased components of a derived-type as the horizontal loop bounds.

NB: this is stacked on top of #249.

github-actions · 2024-03-18T09:31:13Z

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

codecov · 2024-03-18T09:54:29Z

Codecov Report

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

Project coverage is 94.95%. Comparing base (59303e4) to head (aec79bf).

Files	Patch %	Lines
loki/dimension.py	69.23%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
- Coverage   94.95%   94.95%   -0.01%     
==========================================
  Files         153      153              
  Lines       32052    32086      +34     
==========================================
+ Hits        30436    30466      +30     
- Misses       1616     1620       +4

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`95.12% <84.61%> (-0.01%)`	⬇️
transformations	`92.17% <100.00%> (+0.03%)`	⬆️

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.

awnawab · 2024-03-18T15:32:24Z

Can I push a small fix to the dependency transform here or shall I file a separate PR for that?

reuterbal · 2024-03-18T15:54:28Z

Can I push a small fix to the dependency transform here or shall I file a separate PR for that?

I would hold off or make it a separate PR. That transforms has been reshuffled a bit on the new Scheduler branch.

awnawab · 2024-03-25T16:45:30Z

@reuterbal I have rebased and fixed this PR and #249, please let me know when I can push changes to both.

reuterbal · 2024-03-26T10:29:55Z

@reuterbal I have rebased and fixed this PR and #249, please let me know when I can push changes to both.

I've merged the backlog of already reviewed PRs now, so now is likely a good point to rebase your PRs. Many thanks!

awnawab · 2024-04-12T07:20:54Z

This has a conflict now with #270, so I am turning this into a draft until that is reviewed and merged.

…iven

…zation

reuterbal

Thanks, this looks great!

I've left a few comments but no fundamental show stoppers.

reuterbal · 2024-04-22T11:46:15Z

loki/dimension.py

+    bounds_aliases : list or tuple of strings
+        String representations of alternative bounds variables that are
+        used to define loop ranges.


This is really great, I was missing that functionality (and hacked around in the Fortran instead... 😬 )

reuterbal · 2024-04-22T11:50:39Z

loki/tools/util.py

+def resolve_typebound_var(name, variable_map):
+    """
+    A small convenience utility to resolve type-bound variables.
+
+    Parameters
+    ----------
+    name : str
+        The full name of the variable to be resolved, e.g., a%b%c%d.
+    variable_map : dict
+        A map of the variables defined in the current scope.
+    """
+
+    name_parts = name.split('%', maxsplit=1)
+    if (var := variable_map.get(name_parts[0], None)) and len(name_parts) > 1:
+        var = var.get_derived_type_member(name_parts[1])
+    return var


I'd prefer that an expression-specific utility like this lives in the loki.expression or loki.ir package. Could we not make this a method of Scope, for example, with the option to provide the variable_map as an optional argument. It could by default be initialized to self.variable_map but for frequent calls, providing as an argument speeds things up.

Ah yes, that would be much cleaner, thanks! I assume you mean ProgramUnit though rather than Scope, because that is where the variables and variable_map are computed?

Yes, thanks, ProgramUnit would be the right place.

reuterbal · 2024-04-22T12:03:57Z

transformations/transformations/single_column_base.py

+        variables = routine.variables
+        bounds = as_tuple(horizontal.bounds[0])
+        bounds += as_tuple(horizontal.bounds[1])
+        try:
+            if bounds[0] not in variables:
+                raise RuntimeError(f'No horizontal start variable found in {routine.name}')
+            if bounds[1] not in variables:
+                raise RuntimeError(f'No horizontal end variable found in {routine.name}')
+        except RuntimeError as exc:
+            if horizontal._bounds_aliases:
+                bounds = as_tuple(horizontal._bounds_aliases[0])
+                bounds += as_tuple(horizontal._bounds_aliases[1])
+            if bounds[0].split('%', maxsplit=1)[0] not in variables:
+                raise RuntimeError(f'No horizontal start variable found in {routine.name}') from exc
+            if bounds[1].split('%', maxsplit=1)[0] not in variables:
+                raise RuntimeError(f'No horizontal end variable found in {routine.name}') from exc


This looks a bit long-winded, with the use of exceptions instead of ifs. I would add a method bounds_expressions to Dimension, e.g.:

>>> d = Dimension('test_dim_alias', bounds=('istart', 'iend'), bounds_aliases=('bnds%start', 'bnds%end')) >>> d.bounds_expressions (('istart', 'bnds%start'), ('iend', 'bnds%end'))

and then rewrite the method as:

Suggested change

variables = routine.variables

bounds = as_tuple(horizontal.bounds[0])

bounds += as_tuple(horizontal.bounds[1])

try:

if bounds[0] not in variables:

raise RuntimeError(f'No horizontal start variable found in {routine.name}')

if bounds[1] not in variables:

raise RuntimeError(f'No horizontal end variable found in {routine.name}')

except RuntimeError as exc:

if horizontal._bounds_aliases:

bounds = as_tuple(horizontal._bounds_aliases[0])

bounds += as_tuple(horizontal._bounds_aliases[1])

if bounds[0].split('%', maxsplit=1)[0] not in variables:

raise RuntimeError(f'No horizontal start variable found in {routine.name}') from exc

if bounds[1].split('%', maxsplit=1)[0] not in variables:

raise RuntimeError(f'No horizontal end variable found in {routine.name}') from exc

variable_map = routine.variable_map

bounds = ()

for name, bounds in zip(['start', 'end'], horizontal.bounds_expressions):

for bound in bounds:

if bound in variable_map:

bounds += (bound,)

break

else:

raise RuntimeError(f'No horizontol {name} variable matching {horizontal.bounds_expressions[0]} found in {routine.name}')

Many many thanks for this 🙏 this is much cleaner and simpler than what I original had 👌

reuterbal · 2024-04-22T12:04:51Z

transformations/transformations/single_column_base.py

@@ -275,7 +293,7 @@ def process_kernel(self, routine):
            return

        # check for horizontal loop bounds in subroutine symbol table
-        self.check_horizontal_var(routine, self.horizontal)
+        bounds = self.check_horizontal_var(routine, self.horizontal)


Might be sensible to rename this utility to get_horizontal_bound_var or something similar? Also, instead of routine you could directly pass the variable_map, I think.

Yes I agree with the name change, but I'll keep the routine as an argument because I would like to also print the routine name to have a more informative error message.

reuterbal · 2024-04-22T13:32:25Z

transformations/transformations/single_column_coalesced_vector.py

                demote_locals = item.config.get('demote_locals', self.demote_local_arrays)
-            self.process_kernel(routine, demote_locals=demote_locals)
+                preserve_arrays = item.config.get('preserve_arrays', [])
+            self.process_kernel(routine, demote_locals=demote_locals, preserve_arrays=preserve_arrays)


These options should be documented. Would you mind adding a short paragraph in the class docstring?

mlange05

I agree with what @reuterbal pointed out, and have nothing further to add. I can confirm that this runs fine with ec-physics regression.

reuterbal

Many thanks, very happy with this now!

awnawab force-pushed the naan-scc-fixes branch from 98b1f38 to 85ffadc Compare March 18, 2024 10:19

awnawab requested review from mlange05 and reuterbal March 18, 2024 10:54

reuterbal force-pushed the naan-scc-fixes branch from 85ffadc to 3dd95f8 Compare March 25, 2024 11:24

awnawab force-pushed the naan-scc-fixes branch from 3dd95f8 to 71164e1 Compare March 26, 2024 12:07

reuterbal added the rebase required Rebase required for the PR branch to resolve conflicts label Apr 11, 2024

awnawab marked this pull request as draft April 12, 2024 07:21

awnawab force-pushed the naan-scc-fixes branch 6 times, most recently from 77ffee9 to ae61311 Compare April 12, 2024 16:14

awnawab added 8 commits April 19, 2024 12:06

Dimension: add separate bounds and size aliases

364b22e

SCCBase: horizontal bounds vars no longer compulsory if aliases are g…

dca406a

…iven

SCCRevector: use horizontal bounds aliases if vars don't exist

d71de49

SCCDemote: use horizontal size aliases to determine demotion candidates

8aacaaf

SCCDemote: don't demote arrays marked explicitly for preservation

9e6781e

SCCBase: vector notation resolution now works for aliased bounds vars

26f9752

SCCAnnotate: check against size alias when marking arrays for privati…

d12fe4c

…zation

PoolAllocator: switch to typebound var resolution convenience utility

17ab087

awnawab force-pushed the naan-scc-fixes branch from ae61311 to 17ab087 Compare April 19, 2024 12:10

Appease pylint

c679b91

awnawab marked this pull request as ready for review April 19, 2024 14:00

reuterbal removed the rebase required Rebase required for the PR branch to resolve conflicts label Apr 19, 2024

reuterbal requested changes Apr 22, 2024

View reviewed changes

mlange05 reviewed Apr 22, 2024

View reviewed changes

awnawab added 3 commits April 23, 2024 11:16

SCCBase: simplify horizontal loop bounds checking

af7def5

Make typebound var resolution utility a method of ProgramUnit

44127cc

SCCDemote: add documentation for array demotion options

aec79bf

reuterbal approved these changes Apr 23, 2024

View reviewed changes

reuterbal changed the title ~~SCC fixes for SL~~ SCC: Support for bounds aliases and derived type members as bounds Apr 23, 2024

reuterbal added the ready for merge This PR has been approved and is ready to be merged label Apr 23, 2024

reuterbal merged commit c212156 into main Apr 24, 2024
12 checks passed

reuterbal deleted the naan-scc-fixes branch April 24, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCC: Support for bounds aliases and derived type members as bounds #250

SCC: Support for bounds aliases and derived type members as bounds #250

awnawab commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

codecov bot commented Mar 18, 2024 •

edited

Loading

awnawab commented Mar 18, 2024

reuterbal commented Mar 18, 2024

awnawab commented Mar 25, 2024

reuterbal commented Mar 26, 2024

awnawab commented Apr 12, 2024

reuterbal left a comment

reuterbal Apr 22, 2024

reuterbal Apr 22, 2024

awnawab Apr 23, 2024

reuterbal Apr 23, 2024

reuterbal Apr 22, 2024

awnawab Apr 23, 2024

reuterbal Apr 22, 2024

awnawab Apr 23, 2024

reuterbal Apr 22, 2024

mlange05 left a comment

reuterbal left a comment

SCC: Support for bounds aliases and derived type members as bounds #250

SCC: Support for bounds aliases and derived type members as bounds #250

Conversation

awnawab commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

codecov bot commented Mar 18, 2024 • edited Loading

Codecov Report

awnawab commented Mar 18, 2024

reuterbal commented Mar 18, 2024

awnawab commented Mar 25, 2024

reuterbal commented Mar 26, 2024

awnawab commented Apr 12, 2024

reuterbal 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

mlange05 left a comment

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2024 •

edited

Loading