Transformation to call loop transform utilities #430

awnawab · 2024-11-06T13:34:03Z

A transformation to run the loop transform utilities in a specified order, with the aim to place this transformation in scheduler pipelines.

github-actions · 2024-11-06T13:36:21Z

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

codecov · 2024-11-06T14:03:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.14%. Comparing base (c105ed5) to head (d0fb253).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #430   +/-   ##
=======================================
  Coverage   93.13%   93.14%           
=======================================
  Files         200      200           
  Lines       39648    39702   +54     
=======================================
+ Hits        36925    36979   +54     
  Misses       2723     2723

Flag	Coverage Δ
lint_rules	`96.39% <ø> (ø)`
loki	`93.09% <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

Thanks, conceptually this looks good but there's a few improvements that we should do right away to not require breaking the interface afterwards.

I've left inline comments, too, but to summarise:

I would like to have explicit on-switches for each utility, with each off by default
For the RemoveCodeTransformation, we renamed the utilities with do_ prefix. We could use the same pattern here, e.g., call the relevant on-switch loop_fusion and the corresponding utility do_loop_fusion. This makes for a nicer config file appearance (@mlange05, any opinion on this?)
We need a test that verifies the switches and their effectiveness by testing the full permutation matrix of all values

reuterbal · 2024-11-06T20:56:22Z

loki/transformations/tests/test_transform_loop.py

@@ -20,7 +20,7 @@
 )

 from loki.transformations.transform_loop import (
-    loop_interchange, loop_fusion, loop_fission, loop_unroll
+    TransformLoopsTransformation, loop_fusion, loop_fission


Since loop_interchange and loop_unroll are still outward facing API, I think there's a point to be made to use them directly in at least one of the tests.

reuterbal · 2024-11-06T21:02:28Z

loki/transformations/transform_loop.py

+        loop_interchange(routine, project_bounds=self.project_bounds)
+
+        # Fuse loops
+        loop_fusion(routine)
+
+        # Split loops
+        loop_fission(routine, promote=self.promote, warn_loop_carries=self.warn_loop_carries)
+
+        # Unroll loops
+        loop_unroll(routine, warn_iterations_length=self.warn_iterations_length)


I would prefer having off switches for the individual loop transformations, with them disabled by default the same way we have this in the RemoveCodeTransformation:

loki/loki/transformations/remove_code.py

Lines 103 to 110 in c998cda

if self.remove_marked_regions:

do_remove_marked_regions(

routine, mark_with_comment=self.mark_with_comment

)

# Apply Dead Code Elimination

if self.remove_dead_code:

do_remove_dead_code(routine, use_simplify=self.use_simplify)

Each of the utilities is on the more costly side of transformations, and if one of them is causing problems it's also handy to selectively switch them on or off.

We should then also add a test that is parameterised such that each is switched on/off and that verifies that only the respective transformation is then active.

reuterbal · 2024-11-06T21:05:03Z

loki/transformations/transform_loop.py

+            self, project_bounds=False, promote=True, warn_loop_carries=True,
+            warn_iterations_length=True


Without the loop transformation context these options are a little hard to map mentally. I suggest we prefix them according to the transformation they belong to:

Suggested change

self, project_bounds=False, promote=True, warn_loop_carries=True,

warn_iterations_length=True

self, interchange_project_bounds=False,

fission_auto_promote=True, fission_warn_loop_carries=True,

unroll_warn_iterations_length=True

…m utilities

awnawab · 2024-11-08T12:45:44Z

Many thanks @reuterbal for the detailed review and feedback 🙏 I've implemented all of your suggestions, please have a look again whenever you can.

reuterbal

Thanks so much for applying these changes. I think that makes the interface much more coherent!

I have a final suggestion for how to also generalize the test a bit more, because the efficacy of combined transformations is still missing at the moment.

reuterbal · 2024-11-08T12:50:53Z

loki/transformations/tests/test_transform_loop.py

+@pytest.mark.parametrize('loop_trafo', ['loop_interchange', 'loop_fusion', 'loop_fission',
+                                        'loop_unroll'])
+def test_transform_loop_transformation(frontend, loop_trafo):


This will only test one option at a time. What I had in mind was the following:

Suggested change

@pytest.mark.parametrize('loop_trafo', ['loop_interchange', 'loop_fusion', 'loop_fission',

'loop_unroll'])

def test_transform_loop_transformation(frontend, loop_trafo):

@pytest.mark.parametrize('loop_interchange', [False, True])

@pytest.mark.parametrize('loop_fusion', [False, True])

@pytest.mark.parametrize('loop_fission', [False, True])

@pytest.mark.parametrize('loop_unroll', [False, True])

def test_transform_loop_transformation(frontend, loop_interchange, loop_fusion, loop_fission, loop_unroll):

Ah yes, that is a much more comprehensive test, thanks!

reuterbal · 2024-11-08T12:52:20Z

loki/transformations/tests/test_transform_loop.py

+    option = {
+        'loop_interchange': 'loop_interchange' == loop_trafo,
+        'loop_fusion': 'loop_fusion' == loop_trafo,
+        'loop_fission': 'loop_fission' == loop_trafo,
+        'loop_unroll': 'loop_unroll' == loop_trafo
+    }
+    transform = TransformLoopsTransformation(loop_interchange=option['loop_interchange'],
+                                             loop_fusion=option['loop_fusion'],
+                                             loop_fission=option['loop_fission'],
+                                             loop_unroll=option['loop_unroll'])


This would then simplify as

Suggested change

option = {

'loop_interchange': 'loop_interchange' == loop_trafo,

'loop_fusion': 'loop_fusion' == loop_trafo,

'loop_fission': 'loop_fission' == loop_trafo,

'loop_unroll': 'loop_unroll' == loop_trafo

}

transform = TransformLoopsTransformation(loop_interchange=option['loop_interchange'],

loop_fusion=option['loop_fusion'],

loop_fission=option['loop_fission'],

loop_unroll=option['loop_unroll'])

num_loops = len(FindNodes(ir.Loop).visit(routine.body))

num_pragmas = len(FindNodes(ir.Pragma).visit(routine.body))

transform = TransformLoopsTransformation(

loop_interchange=loop_interchange, loop_fusion=loop_fusion,

loop_fission=loop_fission, loop_unroll=loop_unroll'

)

reuterbal · 2024-11-08T13:00:57Z

loki/transformations/tests/test_transform_loop.py

+    pragmas = FindNodes(ir.Pragma).visit(routine.body)
+    assert len(pragmas) == 4
+    assert not any(loop_trafo.replace('_', '-') in pragma.content for pragma in pragmas)
+    assert all(any(opt.replace('_', '-') in pragma.content for pragma in pragmas)
+               for opt in option if not opt == loop_trafo)
+
+    loops = FindNodes(ir.Loop).visit(routine.body)
+    if loop_trafo == 'loop_interchange':
+        assert loops[0].variable == 'j'
+        inner_loops = FindNodes(ir.Loop).visit(loops[0].body)
+        assert inner_loops[0].variable == 'i'
+
+    elif loop_trafo == 'loop_fusion':
+        assigns = FindNodes(ir.Assignment).visit(loops[2].body)
+        assert len(assigns) == 2
+
+    elif loop_trafo == 'loop_fission':
+        assert len(loops) == 7
+
+    elif loop_trafo == 'loop_unroll':
+        assert len(loops) == 5


And the checks could then be generalized as:

Suggested change

pragmas = FindNodes(ir.Pragma).visit(routine.body)

assert len(pragmas) == 4

assert not any(loop_trafo.replace('_', '-') in pragma.content for pragma in pragmas)

assert all(any(opt.replace('_', '-') in pragma.content for pragma in pragmas)

for opt in option if not opt == loop_trafo)

loops = FindNodes(ir.Loop).visit(routine.body)

if loop_trafo == 'loop_interchange':

assert loops[0].variable == 'j'

inner_loops = FindNodes(ir.Loop).visit(loops[0].body)

assert inner_loops[0].variable == 'i'

elif loop_trafo == 'loop_fusion':

assigns = FindNodes(ir.Assignment).visit(loops[2].body)

assert len(assigns) == 2

elif loop_trafo == 'loop_fission':

assert len(loops) == 7

elif loop_trafo == 'loop_unroll':

assert len(loops) == 5

loops = FindNodes(ir.Loop).visit(routine.body)

pragmas = FindNodes(ir.Pragma).visit(routine.body)

if loop_interchange:

num_pragmas -= 1

assert not any('loop-interchange' in pragma.content for pragma in pragmas)

assert loops[0].variable == 'j'

assert FindNodes(ir.Loop).visit(loops[0].body)[0].variable == 'i'

if loop_fusion:

num_pragmas -= 1

assert not any('loop-fusion' in pragma.content for pragma in pragmas)

num_loops -= 1

assert len(FindNodes(ir.Assignment).visit(loops[2].body)) == 2

if loop_fission:

num_pragmas -= 1

assert not any('loop-fission' in pragma.content for pragma in pragmas)

num_loops += 1

if loop_unroll:

num_pragmas -= 1

assert not any('loop-unroll' in pragma.content for pragma in pragmas)

num_loops -= 1

assert len(pragmas) == num_pragmas

assert len(loops) == num_loops

reuterbal

Many thanks, gtg!

awnawab requested review from mlange05 and reuterbal November 6, 2024 13:34

reuterbal requested changes Nov 6, 2024

View reviewed changes

awnawab added 3 commits November 8, 2024 13:43

TransformLoopsTransformation: add trafo to call various loop transfor…

2bf5cc3

…m utilities

TransformLoopsTransformation: make individual components optional

ca204e6

TransformLoopsTransformation: clarify transformation options

1437de6

awnawab force-pushed the naan-loop-transformation branch from c998cda to 1437de6 Compare November 8, 2024 12:43

Update example notebook to use do_loop_fusion

f3032e2

reuterbal requested changes Nov 8, 2024

View reviewed changes

TransformLoopsTransformation: rewrite test as matrix of sub-components

d0fb253

awnawab force-pushed the naan-loop-transformation branch from 966e73d to d0fb253 Compare November 8, 2024 13:27

reuterbal approved these changes Nov 8, 2024

View reviewed changes

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

reuterbal merged commit 7349e2c into main Nov 8, 2024
13 checks passed

reuterbal deleted the naan-loop-transformation branch November 8, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformation to call loop transform utilities #430

Transformation to call loop transform utilities #430

awnawab commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

reuterbal left a comment

reuterbal Nov 6, 2024

reuterbal Nov 6, 2024

reuterbal Nov 6, 2024

awnawab commented Nov 8, 2024

reuterbal left a comment

reuterbal Nov 8, 2024

awnawab Nov 8, 2024

reuterbal Nov 8, 2024

reuterbal Nov 8, 2024

reuterbal left a comment

	if self.remove_marked_regions:
	do_remove_marked_regions(
	routine, mark_with_comment=self.mark_with_comment
	)

	# Apply Dead Code Elimination
	if self.remove_dead_code:
	do_remove_dead_code(routine, use_simplify=self.use_simplify)

		self, project_bounds=False, promote=True, warn_loop_carries=True,
		warn_iterations_length=True

-            self, project_bounds=False, promote=True, warn_loop_carries=True,
-            warn_iterations_length=True
+            self, interchange_project_bounds=False,
+            fission_auto_promote=True, fission_warn_loop_carries=True,
+            unroll_warn_iterations_length=True

-@pytest.mark.parametrize('loop_trafo', ['loop_interchange', 'loop_fusion', 'loop_fission',
-                                        'loop_unroll'])
-def test_transform_loop_transformation(frontend, loop_trafo):
+@pytest.mark.parametrize('loop_interchange', [False, True])
+@pytest.mark.parametrize('loop_fusion', [False, True])
+@pytest.mark.parametrize('loop_fission', [False, True])
+@pytest.mark.parametrize('loop_unroll', [False, True])
+def test_transform_loop_transformation(frontend, loop_interchange, loop_fusion, loop_fission, loop_unroll):

Transformation to call loop transform utilities #430

Transformation to call loop transform utilities #430

Conversation

awnawab commented Nov 6, 2024

github-actions bot commented Nov 6, 2024

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Nov 6, 2024

Choose a reason for hiding this comment

reuterbal Nov 6, 2024

Choose a reason for hiding this comment

reuterbal Nov 6, 2024

Choose a reason for hiding this comment

awnawab commented Nov 8, 2024

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal Nov 8, 2024

Choose a reason for hiding this comment

awnawab Nov 8, 2024

Choose a reason for hiding this comment

reuterbal Nov 8, 2024

Choose a reason for hiding this comment

reuterbal Nov 8, 2024

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2024 •

edited

Loading