Marked region removal and general code removal transformation #276
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/276/index.html |
mlange05
force-pushed
the
naml-code-removal-skip
branch
2 times, most recently
from
d1aa9be to
cc8b63a
Compare
|
Clean with EC-physics regression. |
reuterbal
added
the
rebase required
mlange05
force-pushed
the
naml-code-removal-skip
branch
2 times, most recently
from
ab4f5f9 to
fd8b8d5
Compare
mlange05
removed
the
rebase required
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
=======================================
Coverage 92.88% 92.89%
=======================================
Files 102 102
Lines 18253 18291 +38
=======================================
+ Hits 16954 16991 +37
- Misses 1299 1300 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * :method:`do_remove_calls` | ||
| * :method:`do_remove_marked_regions` | ||
| * :method:`do_remove_dead_code` |
There was a problem hiding this comment.
The markup is not producing the right appearance: https://sites.ecmwf.int/docs/loki/276/loki.transform.transform_remove_code.html#module-loki.transform.transform_remove_code
I think the keyword to create the link is :meth:, or simply :any: should work as well.
Not sure why the list isn't rendered as such, maybe requires an empty line before the first item?
| """ | ||
| :any:`Transformer` class that removes provably unreachable code paths. | ||
|
|
||
| The pirmary modification performed is to prune individual code branches |
There was a problem hiding this comment.
Typo: pirmary -> primary
| if condition == 'True': | ||
| return body | ||
|
|
||
| if condition == 'False': | ||
| return else_body |
There was a problem hiding this comment.
I realise that this has just been moved from the old module, but this comparison against a string put me off for a second... This is using the implicit String comparison to evaluate the condition? Would we want to account for something like IF(1)?
There was a problem hiding this comment.
Yes, this is actually intentional, as IF(1) is basically illegal in a Fortran-context (strong typing; 1 is not a logical) The visual oddness comes from "True" actually being our internal representation of .true. (the fortran intrinsic logical value), and that is the only thing we should match in a condition, since everything else should evaluate to that via simplify (eg. result of a logical operator will be a logical, never a numerical value).
There was a problem hiding this comment.
Great, thanks - I think I did not actually know that :)
| to specific named subroutines. | ||
|
|
||
| This :any:`Transformer` will by default also remove the enclosing | ||
| inline-conditional when encountering calls of the form ```if |
There was a problem hiding this comment.
A stray '`' here and after the stop.
This is a simple utility to remove code regions marked with ``!$loki remove`` pragma regions from a subroutine body.
This is similar in scope to the previous "external" utility in the `transformations` package, and borrows some of its testing.
mlange05
force-pushed
the
naml-code-removal-skip
branch
from
866c4e6 to
bf9cb98
Compare
|
Hi @reuterbal just a quick heads-up: there was some cross-fire between PR #271 and this one, causing a test failure for scheduler / pipeline configs. I've rebased over latest main and adjusted accordingly. Only the top two commits should be new. Hope this works now. |
|
Many thanks for catching the rebase fall-out! As discussed offline, I've done a little touch-up on the docstrings, otherwise this should be good to go! |
reuterbal
added
the
ready for merge
This PR combines two closely related features:
Transformationfor inclusion in more complex pipelines.The new pragma-based region removal will pretty much do as it says: Upon encountering a region marked with
!$loki remove/!$loki end removeit will simply remove the entire code region.The new
loki.transfom.transform_remove_codebasically includes the previous dead code elimination (renamed), and combines it with the new region removal and a re-write of theRemoveCallsTransformation. The latter is now a as a singleTransformerwith a wrapper routine that allows it to be applied toSubroutineobjects. All three utilities are then provided as a singleRemoveCodeTransformationfor use with theSchedulerand batch-processing engine.Since the wrapper routines for these utilities would otherwise create name clashes with the respective flags of the overarching
Transformation, we adopt thedo_remove_xxxnaming convention, as discussed offline.In some more detail:
loki.transform.transform_dead_code=>loki.transform.transform_remove_codeRemoveMarkedRegionsTransformeranddo_remove_marked_regionswrapperRemvoeCallsTransformeranddo_remove_callswrappereleminate_dead_codetoRemoveDeadCodeTransformeranddo_remove_dead_codeRemoveCodeTransformationin loki-transform and remove the oldRemoveCallsTransformationfromtransformationstransformations.utility_rouines=>transformations.drhook, as it's the last custom utility left in there.