Recursive inlining via InlineTransform and associated fixes #205

mlange05 · 2024-01-09T08:23:44Z

This PR separates the various styles of source-inlining (elementals, internals, pragma-marked) into a dedicated Transformation class that can be configured independently. This key motivation for this is that this allows us to perform recursive multi-step inlining, where individual inlining steps are performed at the leaves first and then propagated up by traversing the Scheduler tree in reverse order.

As these steps where previously baked into the SCCBaseTransformation several of these steps have now been removed. Due to complex interplay in the ec-physics regression, things like associate resolution and dead-code-elimination have to now also be performed with the inlining steps. Until we have proper pipeline configuration, I have chosen to go the simple route and simply lump these in with the InlineTransformation, but would appreciate feedback on this. This refactoring also has show some (severe!) problems with the previous "elemental function" inliner, which are also fixed in this PR.

Edit: As discussed offline, I've also added a first draft of a "housekeeping" transformation (SanitiseTransformation), for a easily configurable code harmonisation utility pass. I plan to extend this as we progress through the trafor cleanup and consolidation.

Edit: Turns out one of the small fixes actually has a some far-reaching consequences: The elemental inliner was originally not checking for true elemental functions, and it now does(!), but the problem is that our OFP frontend cannot deal with prefix keywords. As the elemental inliner is needed for C-transpilation though, this means a bunch of tests, including CLOUDSC, have to be disabled for OFP. I've had a go at retro-fitting this feature, but it's quite involed due to the broken scoping around type nodes in the OFP AST. I propose dropping OFP entirely now, but I'll track that as a separate issue.

github-actions · 2024-01-09T08:26:09Z

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

codecov · 2024-01-15T11:05:58Z

Codecov Report

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

Comparison is base (44fcf08) 92.26% compared to head (63a4434) 92.26%.

Files	Patch %	Lines
loki/transform/transform_inline.py	95.65%	2 Missing ⚠️
loki/types.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
- Coverage   92.26%   92.26%   -0.01%     
==========================================
  Files          96       95       -1     
  Lines       17132    17176      +44     
==========================================
+ Hits        15807    15847      +40     
- Misses       1325     1329       +4

Flag	Coverage Δ
lint_rules	`96.22% <ø> (ø)`
loki	`92.24% <96.66%> (+<0.01%)`	⬆️
transformations	`91.46% <100.00%> (-0.06%)`	⬇️

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

This looks great and is much appreciated!

I left a few comments while browsing the changes, most of which are not strictly necessary to act upon other than some massaging of docstrings (marking this as Request changes for that). One thing to consider, though, would be to add a test for the new SanitiseTransformation to the Loki core's test base. It's currently only being invoked from the tests in the transformations sub-package.

reuterbal · 2024-01-23T07:51:31Z

loki/transform/transform_inline.py

 ]


+class InlineTransformation(Transformation):
+    """
+    :any:`Transformation` object to apply several types of source inlining


<pedantic>

Suggested change

:any:`Transformation` object to apply several types of source inlining

:any:`Transformation` class to apply several types of source inlining

</pedantic>

reuterbal · 2024-01-23T07:54:41Z

loki/transform/transform_inline.py

+    inline_constants : bool
+        Replace instances of variables with known constant values by
+        :any:`Literal`; default: False.
+    inline_elementals : bool
+        Replaces :any:`InlineCall` expression to elemental functions
+        with the called function's body; default: True.
+    inline_internals : bool
+        Perform internal procedure inlining via
+        :method:`inline_internal_procedures`; default: False.
+    inline_marked : bool
+        Inline :any:`Subroutine` objects marked by pragma annotations;
+        default: True.
+    eliminate_dead_code : bool
+        Perform dead code elimination, where unreachable branches are
+        trimmed from the code; default@ True
+    allowed_aliases : tuple or list of str or :any:`Expression`, optional
+        List of variables that will not be renamed in the parent scope during
+        internal and pragma-driven inlining.
+    remove_imports : bool
+        Strip unused import symbols after pragma-inlining (optional, default: True)
+    external_only : bool, optional
+        Do not replace variables declared in the local scope when
+        inlining constants (default: True)


From a readability point of view, it could be useful to provide a cross-link to the respective utility function, e.g.:

inline_constants : bool Replace instances of variables with known constant values by :any:`Literal` (see :any:`inline_constant_parameters`); default: False.

That'll generate a direct link in the HTML docs to read up on the specific utility's behaviour.

reuterbal · 2024-01-23T07:55:23Z

loki/transform/transform_inline.py

+    Notes
+    -----
+    The ``.type.initial`` property is used to derive the replacement
+    value,a which means for symbols imported from external modules,
+    the parent :any:`Module` needs to be supplied in the
+    ``definitions`` to the constructor when creating :param:`routine`.
+
+    Variables that are replaced are also removed from their
+    corresponding import statements, with empty import statements
+    being removed alltogether.


This needs to come before Parameters; Sphinx disregards anything after Parameters and/or Returns.

reuterbal · 2024-01-23T08:06:48Z

loki/transform/transform_inline.py

@@ -179,7 +268,10 @@ def inline_elemental_functions(routine):

    exprmap = {}
    for call in FindInlineCalls().visit(routine.body):
-        if call.procedure_type is not BasicType.DEFERRED:
+        if call.procedure_type is BasicType.DEFERRED:


Conceptual question, not necessary to act upon here: Going forward, should we try to include debug output (or a new "optimisation report"-like output stream) in situations like this, where a transformation is not applied because an assumption isn't met (here: missing enrichment information).

Yes, I'm planning an overhaul of the logging from transformations once the configuration changes are more complete (so we can select log-leve per trafo, etc.).

Possibly worth capturing via an issue?

reuterbal · 2024-01-23T10:45:32Z

loki/transform/transform_inline.py

@@ -193,7 +285,7 @@ def inline_elemental_functions(routine):
    # Remove all module imports that have become obsolete now
    import_map = {}
    for im in FindNodes(Import).visit(routine.spec):
-        if all(hasattr(s, 'type') and s.type.dtype in removed_functions for s in im.symbols):
+        if im.symbols and all(hasattr(s, 'type') and s.type.dtype in removed_functions for s in im.symbols):


I think the hasattr(s, 'type') should no longer be necessary - that is a remnant of the times when we represented the symbol list in Import with names as strings.

Will try, but need to rebase this over ec-phys regression...

reuterbal · 2024-01-23T10:51:24Z

transformations/transformations/single_column_coalesced.py

-            # When inlining we allow the horizontal dimension to alias, so that
-            # the de/re-vectorisation captures the shared vector dimension.
-            inline_marked_subroutines(routine, allowed_aliases=(self.horizontal.index,))
-
        # Associates at the highest level, so they don't interfere
        # with the sections we need to do for detecting subroutine calls
        resolve_associates(routine)


What's the reason for not including resolve_associates in the SanitiseTransformation?

Oversight, now fixed (I hope).

Scratch that - better answer is: Because EC-physics regression breaks! 😭 There seems to be some unfortunate side-effects with the SCC-vector demotion if this is executed before the Inliner - which this implicitly does.

I'm planning a more genera refactoring of the individual steps where this and some other steps would be brought forward into the "sanitise pass". Would you mind if we hold back on this change until then, @reuterbal ?

Sure, I only asked for the reason, and that's a good enough one ;-)

mlange05 · 2024-01-24T12:57:31Z

Ok, I rebased and pushed the fixes to the suggestions - many thanks @reuterbal . I'll have to rebase/re-run this with ec-phys regression, just to make sure, but will ping here once confirmed.

As the removal of calls can make import statements obsolete, we add an option to remove this. This can help with keeping the call-tree clean.

To separately configure and apply recursive source inlining for different entities, we add this Transformation and according test.

This so far includes resolving associates and sequence associations, but could be extended into a general-purpose input unification pipeline.

mlange05 · 2024-01-25T16:29:43Z

Ok, rebased, CI happy and confirmed with EC-physics regression. I've also added test for the new trafo and addressed all comments, except the breakage one. I think this is ready for another look, @reuterbal

reuterbal

Many thanks, changes look good to me and everything has been addressed. GTG!

reuterbal · 2024-01-25T20:35:51Z

loki/transform/transform_inline.py

@@ -179,7 +268,10 @@ def inline_elemental_functions(routine):

    exprmap = {}
    for call in FindInlineCalls().visit(routine.body):
-        if call.procedure_type is not BasicType.DEFERRED:
+        if call.procedure_type is BasicType.DEFERRED:


reuterbal · 2024-01-25T20:39:43Z

transformations/transformations/single_column_coalesced.py

-            # When inlining we allow the horizontal dimension to alias, so that
-            # the de/re-vectorisation captures the shared vector dimension.
-            inline_marked_subroutines(routine, allowed_aliases=(self.horizontal.index,))
-
        # Associates at the highest level, so they don't interfere
        # with the sections we need to do for detecting subroutine calls
        resolve_associates(routine)


Sure, I only asked for the reason, and that's a good enough one ;-)

mlange05 requested a review from reuterbal January 9, 2024 08:23

mlange05 force-pushed the naml-pragma-inline-remove-imports branch 7 times, most recently from bac2a6d to 5a2c7ed Compare January 15, 2024 10:37

mlange05 force-pushed the naml-pragma-inline-remove-imports branch 2 times, most recently from 453347b to 071e9dc Compare January 16, 2024 06:30

mlange05 marked this pull request as ready for review January 16, 2024 07:54

mlange05 force-pushed the naml-pragma-inline-remove-imports branch from 071e9dc to ef8a5cd Compare January 16, 2024 07:55

reuterbal requested changes Jan 23, 2024

View reviewed changes

mlange05 force-pushed the naml-pragma-inline-remove-imports branch from ef8a5cd to 3e86a96 Compare January 24, 2024 11:06

mlange05 force-pushed the naml-pragma-inline-remove-imports branch 2 times, most recently from e44f43f to 654193e Compare January 25, 2024 15:25

mlange05 added 11 commits January 25, 2024 16:43

TransformInline: Add option to remove unused imports to pragma inliner

aa6c5be

As the removal of calls can make import statements obsolete, we add an option to remove this. This can help with keeping the call-tree clean.

TransformInline: Add InlineTransformation for batch processing

bc9d112

To separately configure and apply recursive source inlining for different entities, we add this Transformation and according test.

Loki-transform: Perform source inlining via new InlineTransformation

dc910e1

InlineTransform: Resolve associations and skip unattached calls

aa57690

TransformInline: Also remove interface header imports

f873719

TransformInline: Fix elemental function inlining

655ee8f

SanitiseTransform: Create "housekeeping" transformation for scheduler

61dca1d

This so far includes resolving associates and sequence associations, but could be extended into a general-purpose input unification pipeline.

SCC: Fix SCC + sequence-assoc + internal-inlining test

00a462d

OFP: Disable OFP for regression tests

be9520a

TransformInline: Fix docstrings and add cross-linkage

038162d

InlineElementals: Remove redundant check for .type in import symbols

d6ba0b8

mlange05 added 2 commits January 25, 2024 16:43

TransformSanitise: Merge tests for sanitise transformation

5705036

TransformSanitise: Add small test for Transformation class

63a4434

mlange05 force-pushed the naml-pragma-inline-remove-imports branch from 654193e to 63a4434 Compare January 25, 2024 15:43

reuterbal mentioned this pull request Jan 25, 2024

Improved logging from transformations #220

Open

reuterbal approved these changes Jan 25, 2024

View reviewed changes

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

reuterbal merged commit 903798f into main Jan 26, 2024
12 checks passed

reuterbal deleted the naml-pragma-inline-remove-imports branch January 26, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive inlining via InlineTransform and associated fixes #205

Recursive inlining via InlineTransform and associated fixes #205

mlange05 commented Jan 9, 2024 •

edited

Loading

github-actions bot commented Jan 9, 2024

codecov bot commented Jan 15, 2024 •

edited

Loading

reuterbal left a comment

reuterbal Jan 23, 2024

reuterbal Jan 23, 2024

mlange05 Jan 24, 2024

reuterbal Jan 23, 2024

reuterbal Jan 23, 2024

mlange05 Jan 24, 2024

mlange05 Jan 24, 2024

reuterbal Jan 25, 2024

reuterbal Jan 23, 2024

mlange05 Jan 24, 2024

reuterbal Jan 23, 2024

mlange05 Jan 24, 2024

mlange05 Jan 24, 2024

reuterbal Jan 25, 2024

mlange05 commented Jan 24, 2024

mlange05 commented Jan 25, 2024

reuterbal left a comment

reuterbal Jan 25, 2024

reuterbal Jan 25, 2024

	:any:`Transformation` object to apply several types of source inlining
	:any:`Transformation` class to apply several types of source inlining

Recursive inlining via InlineTransform and associated fixes #205

Recursive inlining via InlineTransform and associated fixes #205

Conversation

mlange05 commented Jan 9, 2024 • edited Loading

github-actions bot commented Jan 9, 2024

codecov bot commented Jan 15, 2024 • edited Loading

Codecov Report

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

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 commented Jan 24, 2024

mlange05 commented Jan 25, 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

mlange05 commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 15, 2024 •

edited

Loading