Extract: Improved region-outlining for complex procedures #412

mlange05 · 2024-10-19T18:49:02Z

This change brings various improvements to enable the use of outine_pragma_regions for more complex code regions. This is intended to eventually allow us to combine pragma-based extraction with auto-parallelisation to re-rewrite complex control-flow sections.

In particular, this PR refactors the utility structure to allow invocations per-region, deals with derived type symbols and arguments, and imposes an ordering on extracted argument signatures. There are also other small improvements added, as well as a utility to remove unused imports and import symbols.

In more detail:

Use of Transformer, instead of the more complex NestedMaskedTransformer to replace PragmaRegion nodes
Separation of the outer loop over subroutines from the specific single call-subroutine extraction for a single region
A new utility to remove unused imports and trim unused symbols from import statements within a routine
Ability to handle C-style imports when pragma-outlining sburoutines
Resolving explicitly requested argument symbols (via the "intent" notation) from the parent routine. This allows explicitly requesting arguments that might not be used in the body, but are necessary for shape info in the spec of the extracted routine.
Handling of derived-type symbols in extracted body/routine, in particular ensuring that only the parent is added to the extracted routine signature
Additional tests for derived-type arguments and associated symbols in extracted bodies/routines. The latter is not strictly needed, but useful to have.
Explicit ordering of arguments for reproducibility - this will use lexicographic base ordering, but put derived-type arguments, before array arguments, before scalar arguments.

github-actions · 2024-10-19T18:51:24Z

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

codecov · 2024-10-19T19:16:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.09%. Comparing base (13aba08) to head (66995cf).
Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   93.07%   93.09%   +0.01%     
==========================================
  Files         198      198              
  Lines       39449    39547      +98     
==========================================
+ Hits        36719    36817      +98     
  Misses       2730     2730

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`93.04% <100.00%> (+0.01%)`	⬆️

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 a valuable expansion of functionality over the original stubs!

I've left a few minor comments that would be good to address but no major showstoppers. However, I think we have a duplication between sanitise_imports and the new utility to remove unused imports.

loki/transformations/extract/outline.py

reuterbal · 2024-11-04T12:47:44Z

loki/transformations/remove_code.py

@@ -332,3 +337,68 @@ def visit_Import(self, o, **kwargs):

        rebuilt = tuple(self.visit(i, **kwargs) for i in o.children)
        return self._rebuild(o, rebuilt)
+
+
+def do_remove_unused_imports(routine):


Is this not redundant to or at least overlapping in functionality with the sanitise_imports utility? I would prefer if we combined the two somehow.

loki/loki/transformations/utilities.py

Lines 281 to 295 in 83c9ebe

def sanitise_imports(module_or_routine):

"""

Sanitise imports by removing unused symbols and eliminating imports

with empty symbol lists.

Note that this is currently limited to imports that are identified to be :class:`Scalar`,

:class:`Array`, or :class:`ProcedureSymbol`.

"""

if isinstance(module_or_routine, Subroutine):

find_and_eliminate_unused_imports(module_or_routine)

elif isinstance(module_or_routine, Module):

used_symbols = set()

for routine in module_or_routine.subroutines:

used_symbols |= find_and_eliminate_unused_imports(routine)

eliminate_unused_imports(module_or_routine, used_symbols)

Good spot had forgotten about the overlap with this. However, the existing utility doesn't work for my use case (and I'd argue Transformer-based is better), but this certainly looks like something that deserves its own PR. If you don't mind, I'll drop this in a local rebase?

This now allows us to call outlining extraction on a single pragma region, while the overarching utility uses this in its own pragma loop.

Need to explicit give an empty tuple for kwargument when creating the CallStatement, as dataflow analysis will iterate it.

…lining Co-authored-by: Balthasar Reuter <[email protected]>

mlange05 · 2024-11-07T13:37:24Z

@reuterbal Ok, I've rebased out the import sanitisation utility for now and addressed all other comments in the top two commits, as we as a fresh rebase over main. Please have another look.

reuterbal

Excellent, looks good.

I agree on the better implementation of the import sanitisation and happy to sort out the overlap separately!

mlange05 requested a review from reuterbal October 19, 2024 18:49

mlange05 force-pushed the naml-improve-extract-outline branch from 1987b85 to 657550c Compare October 20, 2024 04:27

mlange05 marked this pull request as ready for review October 20, 2024 04:59

reuterbal requested changes Nov 4, 2024

View reviewed changes

mlange05 and others added 11 commits November 7, 2024 13:31

Extract: Use Transformer in outline_pragma_regions

032eb4c

Extract: Add outline_region for single region extraction

53de1f8

This now allows us to call outlining extraction on a single pragma region, while the overarching utility uses this in its own pragma loop.

Extract: Bug fix for outline_pragma_regions

99a17ea

Need to explicit give an empty tuple for kwargument when creating the CallStatement, as dataflow analysis will iterate it.

Extract: Add C-import special-case to routine extraction

58a8e7e

Extract: Allow derived-type accesses in routine extraction

9082e63

Extract: Resolve intent overrides from caller when extracting

5d81e25

Extract: Add test for outlininig extraction with associates

e2aa27f

Extract: Order declarations when extracting routines

fd0242f

Extract: Ensure correct re-scoping after cloning arguments

76dc83d

Extract: Better use of internal Loki API in outline_pragma_regions

736948e

Extract: Better way to filter derived types in arg reordering for out…

66995cf

…lining Co-authored-by: Balthasar Reuter <[email protected]>

mlange05 force-pushed the naml-improve-extract-outline branch from 7b53b7e to 66995cf Compare November 7, 2024 13:33

reuterbal approved these changes Nov 7, 2024

View reviewed changes

reuterbal added the ready to merge This PR has been approved and is ready to be merged label Nov 7, 2024

reuterbal merged commit 19dc642 into main Nov 7, 2024
13 checks passed

reuterbal deleted the naml-improve-extract-outline branch November 7, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract: Improved region-outlining for complex procedures #412

Extract: Improved region-outlining for complex procedures #412

mlange05 commented Oct 19, 2024

github-actions bot commented Oct 19, 2024

codecov bot commented Oct 19, 2024 •

edited

Loading

reuterbal left a comment

reuterbal Nov 4, 2024

mlange05 Nov 7, 2024

mlange05 commented Nov 7, 2024

reuterbal left a comment

	def sanitise_imports(module_or_routine):
	"""
	Sanitise imports by removing unused symbols and eliminating imports
	with empty symbol lists.

	Note that this is currently limited to imports that are identified to be :class:`Scalar`,
	:class:`Array`, or :class:`ProcedureSymbol`.
	"""
	if isinstance(module_or_routine, Subroutine):
	find_and_eliminate_unused_imports(module_or_routine)
	elif isinstance(module_or_routine, Module):
	used_symbols = set()
	for routine in module_or_routine.subroutines:
	used_symbols \|= find_and_eliminate_unused_imports(routine)
	eliminate_unused_imports(module_or_routine, used_symbols)

Extract: Improved region-outlining for complex procedures #412

Extract: Improved region-outlining for complex procedures #412

Conversation

mlange05 commented Oct 19, 2024

github-actions bot commented Oct 19, 2024

codecov bot commented Oct 19, 2024 • edited Loading

Codecov Report

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Nov 4, 2024

Choose a reason for hiding this comment

mlange05 Nov 7, 2024

Choose a reason for hiding this comment

mlange05 commented Nov 7, 2024

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2024 •

edited

Loading