Block-index injection transformations #303

awnawab · 2024-04-29T09:29:43Z

In order to mitigate the cost of creating thread-local copies of data structures, per-block views of fields can be replaced with full field views and the block-index can be inserted into the field pointer.

This is achieved via two transformations:

BlockViewToFieldViewTransformation: A very IFS specific transformation that simply replaces per-block view pointers with full field pointers. It also injects the missing type definitions for FIELD_RANKSUFF_ARRAY types.
BlockIndexInjectTransformation: A general transformation that injects the block-index to all arrays that have a local rank one less than their declared rank

github-actions · 2024-04-29T09:32:18Z

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

codecov · 2024-04-29T10:05:55Z

Codecov Report

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

Project coverage is 95.14%. Comparing base (f3e7d90) to head (6707094).

Files	Patch %	Lines
...oki/transformations/block_index_transformations.py	98.70%	2 Missing ⚠️
...i/transformations/tests/test_block_index_inject.py	98.97%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
+ Coverage   95.12%   95.14%   +0.02%     
==========================================
  Files         165      167       +2     
  Lines       35143    35374     +231     
==========================================
+ Hits        33429    33656     +227     
- Misses       1714     1718       +4

Flag	Coverage Δ
lint_rules	`96.38% <ø> (-0.02%)`	⬇️
loki	`95.12% <99.03%> (+0.02%)`	⬆️

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.

transformations/transformations/block_index_transformations.py

transformations/tests/test_block_index_inject.py

MichaelSt98

Really nice! And I just started to implement some transformation/utilities on top of/utilising the BlockIndexInjectTransformation without encountering any problem.

Nevertheless, some comments regarding some typos and questions/suggestions (probably because I have not fully understood the BlockViewToFieldViewTransformation)

cmake/loki_transform.cmake

transformations/tests/test_block_index_inject.py

MichaelSt98 · 2024-05-07T11:08:24Z

transformations/tests/test_block_index_inject.py

+        paths=(blockview_to_fieldview_code[0],), config=config, seed_routines='driver', frontend=frontend
+    )
+    scheduler.process(BlockViewToFieldViewTransformation(horizontal, global_gfl_ptr=True))
+    scheduler.process(BlockIndexInjectTransformation(blocking))


Could/should we add some tests/assertions to test the result/output of BlockViewToFieldViewTransformation before applying BlockIndexInjectTransformation? Like this only the combination of both transformation is tested?!

Good point, and I'll also use those tests to mop-up the missing lines highlighted by the coverage report. Thanks!

transformations/transformations/block_index_transformations.py

reuterbal

First of all, apologies for the delay in reviewing this.

Many thanks, this looks great and I like the structure with the split into two transformation steps to separate generic and specific functionality.

I've left quite a few comments, some are already a couple of days old and therefore might be outdated by now, but they are mostly about details or documentation.

scripts/loki_transform.py

transformations/transformations/block_index_transformations.py

reuterbal · 2024-05-10T12:02:31Z

transformations/transformations/block_index_transformations.py

+
+        # First get rank mismatched call statement args
+        vmap = {}
+        for call in [call for call in FindNodes(ir.CallStatement).visit(body) if call.name in targets]:


Suggested change

for call in [call for call in FindNodes(ir.CallStatement).visit(body) if call.name in targets]:

for call in FindNodes(ir.CallStatement).visit(body):

if call.name in targets:

transformations/transformations/block_index_transformations.py

loki/transformations/block_index_transformations.py

loki/transformations/tests/test_block_index_inject.py

transformations/transformations/block_index_transformations.py

awnawab · 2024-05-13T15:52:13Z

Thanks again @reuterbal for the derived-type enrichment fix 🙏

reuterbal

Many thanks! Sorry for the kerfuffle around enrichment but I'm glad this solution works as I had a suspicion this was a symptom of an underlying bug.

mlange05

Very happy with this overall, so GTG from me.

I left a few comments about overall structure of the transformations sub-package, but these are merely meant as commentary, as this change highlights the need for further restructuring.

mlange05 · 2024-05-27T03:39:52Z

loki/transformations/block_index_transformations.py

+
+        # Sanitize the subroutine
+        resolve_associates(routine)
+        v_index = SCCBaseTransformation.get_integer_variable(routine, name=self.horizontal.index)


[no action] Note to self, these should be separated out into general utilities. The fact that this field trafo is calling into the SCC stack signals the problem.

mlange05 · 2024-05-27T03:44:41Z

loki/transformations/block_index_transformations.py

+            parent = self.build_ydvars_global_gfl_ptr(parent)
+
+        _type = var.type
+        if 'gfl_ptr' in var.basename.lower():


[no action] This is extremely bespoke to (potentially changing) IFS conventions, which is absolutely fine for now. In the future we will likely need to factor out bespoke "IFS Fields" transformations into a separate sub-package, as we add more to these.

…rafo

…r-layer code

awnawab force-pushed the naan-block-index-inject branch from 7b0f059 to 2bf86eb Compare April 29, 2024 09:35

MichaelSt98 reviewed Apr 29, 2024

View reviewed changes

transformations/transformations/block_index_transformations.py Outdated Show resolved Hide resolved

MichaelSt98 reviewed Apr 29, 2024

View reviewed changes

transformations/tests/test_block_index_inject.py Outdated Show resolved Hide resolved

MichaelSt98 reviewed May 7, 2024

View reviewed changes

awnawab force-pushed the naan-block-index-inject branch from 2bf86eb to 4d4f6ee Compare May 10, 2024 12:13

reuterbal requested changes May 10, 2024

View reviewed changes

awnawab force-pushed the naan-block-index-inject branch 3 times, most recently from f7153f9 to cd8eed3 Compare May 11, 2024 16:50

reuterbal reviewed May 13, 2024

View reviewed changes

loki/transformations/tests/test_block_index_inject.py Outdated Show resolved Hide resolved

loki/transformations/tests/test_block_index_inject.py Show resolved Hide resolved

transformations/transformations/block_index_transformations.py Outdated Show resolved Hide resolved

reuterbal approved these changes May 13, 2024

View reviewed changes

awnawab requested a review from mlange05 May 25, 2024 20:37

mlange05 approved these changes May 27, 2024

View reviewed changes

awnawab added 14 commits May 27, 2024 09:06

Dimension: add index alias attribute

9770a4b

DeprivatiseStructsTrafo: first implementation

925a71b

BlockIndexInjectTransformation: first implementation

460b81b

Change trafo name to UnprivatiseStructsTransformation

1abe60b

SCRIPTS: add unprivatise_structs option to scripts

78e070c

BlockIndexInject: cleanup and fixes

acc16d5

BlockIndexInject: fix call arg rank logic

7d1c5f1

Various improvements to UnprivatiseStructsTrafo and BlockIndexInjectT…

95b7121

…rafo

Add configurable exclude list to unprivatise/blockinject trafos

4114ae6

SCRIPTS: read unprivatise pipeline from config

f67806d

Adapt unprivatise/blockinject trafos so they can also be run on drive…

e60eca6

…r-layer code

Rename file to block_index_transformations.py

9eacc8e

BlockViewToFieldViewTrafo: add documentation

7ffb876

BlockIndexInjectTrafo: add documentation

2712538

awnawab and others added 22 commits May 27, 2024 09:06

Change name of loki-transform script arg

2fcdc08

Make exclude_arrays configurable via the scheduler

dd8f0d4

BlockViewtoFieldViewTrafo: make switch to global gfl_ptr optional

5ce3b84

BlockViewtoFieldViewTrafo: add bailout for routines marked as seq

a750450

Dimension: add index_expressions property

1d474ea

BlockIndexInjectTrafos: add tests

964002b

Appease pylint

b58be6d

BlockIndexInjectTrafo: add support for call statement kwargs

facaca9

BlockIndexTrafos: fix typos and cover untested lines

4fc6af1

Rebase cleanup

729120c

InjectBlockIndexTransformation: rename trafo

9a8df04

block_index_trafos: remove trafo_data key from constructor args

d31043a

Enrich derived-type variable declarations

7e67321

BlockViewToFieldViewTrafo: update documentation

07366c0

block_index_trafos: misc cleanup

f30b376

SCRIPTS: remove BLOCKVIEW_TO_FIELDVIEW CLI arg

8787f0d

Angry linting gods

212677b

SCRIPTS: sanitize mode string for custom pipelines

e71adc4

Change FindVariables searches to non-unique to restore SL functionality

1e05c1b

Add a test for derived type enrichment

2c01d69

Fix enrichment of derived type dtypes for local variables

998d1f4

block_index_trafos: change scope of test config fixture to function

9ae7b50

awnawab force-pushed the naan-block-index-inject branch from eea26f5 to 9ae7b50 Compare May 27, 2024 07:06

Appease extra fussy upgraded linter

6707094

reuterbal merged commit 77114a9 into main May 28, 2024
12 checks passed

reuterbal deleted the naan-block-index-inject branch May 28, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block-index injection transformations #303

Block-index injection transformations #303

awnawab commented Apr 29, 2024

github-actions bot commented Apr 29, 2024

codecov bot commented Apr 29, 2024 •

edited

Loading

MichaelSt98 left a comment

MichaelSt98 May 7, 2024

awnawab May 10, 2024

reuterbal left a comment

reuterbal May 10, 2024

awnawab commented May 13, 2024 •

edited

Loading

reuterbal left a comment

mlange05 left a comment

mlange05 May 27, 2024

mlange05 May 27, 2024

	for call in [call for call in FindNodes(ir.CallStatement).visit(body) if call.name in targets]:
	for call in FindNodes(ir.CallStatement).visit(body):
	if call.name in targets:

Block-index injection transformations #303

Block-index injection transformations #303

Conversation

awnawab commented Apr 29, 2024

github-actions bot commented Apr 29, 2024

codecov bot commented Apr 29, 2024 • edited Loading

Codecov Report

MichaelSt98 left a comment

Choose a reason for hiding this comment

MichaelSt98 May 7, 2024

Choose a reason for hiding this comment

awnawab May 10, 2024

Choose a reason for hiding this comment

reuterbal left a comment

Choose a reason for hiding this comment

reuterbal May 10, 2024

Choose a reason for hiding this comment

awnawab commented May 13, 2024 • edited Loading

reuterbal left a comment

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

mlange05 May 27, 2024

Choose a reason for hiding this comment

mlange05 May 27, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 29, 2024 •

edited

Loading

awnawab commented May 13, 2024 •

edited

Loading