Frontend Transformer optimisation #241
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/241/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
=======================================
Coverage 92.47% 92.48%
=======================================
Files 95 95
Lines 17473 17485 +12
=======================================
+ Hits 16159 16171 +12
Misses 1314 1314
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Confirmed clean with ec-physics regression. |
There was a problem hiding this comment.
Thanks for looking into this! In principle uncontroversial and the use of inplace transformers is very sensible here.
I'm a bit puzzled about the comments inlining implementation - I'm not convinced it still does what it is intended to do. Just want to make sure we don't break this functionality accidentally.
loki/frontend/util.py
Outdated
| def visit_tuple(self, o, **kwargs): | ||
| pairs = PatternFinder(pattern=(Assignment, Comment)).visit(o) | ||
| pairs += PatternFinder(pattern=(VariableDeclaration, Comment)).visit(o) | ||
| pairs += PatternFinder(pattern=(ProcedureDeclaration, Comment)).visit(o) | ||
|
|
||
| for pair in pairs: | ||
| # Comment is in-line and can be merged | ||
| if pair[0].source and pair[1].source: | ||
| if pair[1].source.lines[0] == pair[0].source.lines[1]: | ||
| new = pair[0]._rebuild(comment=pair[1]) | ||
| o = replace_windowed(o, pair, new) | ||
| return o |
There was a problem hiding this comment.
Slightly confused by the implementation:
Undoubtedly, the performance gain comes here from not using NestedTransformer and instead doing in-place updates for the found nodes. However, I don't understand why this is implemented as a Transformer, and not convinced it does always behave as expected?
It shortcuts the recursion in the Transformer upon the first tuple - which will in the frontend use-case usually be the Section.body for something like spec, body in a subroutine. But the PatternFinder is then the only place to actually recurse into the IR. Does the replacement really work as expected for inline comments further nested into the IR than the outermost tuple?
Unless we're explicitly testing for this, it wouldn't upset anything because it just means the inline comments continue to live as standalone comments. So, I'm wondering whether the effect is merely that we don't convert the comments to inline comments at all anymore?
There was a problem hiding this comment.
Wow, very good spot! You are totally right that
a) this isn't tested properly!
b) We recurse in PatternFinder, which we don't need anymore, but not in visit_tuple
I've tried to address both in two commits I just pushed, and with those the behaviour seems correct, and performance benefits stay as before. Please have another look (once CI agrees)
There was a problem hiding this comment.
Funny, this also fixed the test_ir_graph, so that I could remove the commit that adjusts the node/edge counting test (hence recent force-push).
mlange05
force-pushed
the
naml-frontend-transformers
branch
from
dd6db97
to
e77e1cf
Compare
Note: To manage the complexity of what became quite a complex operation, I'm separating the simple steps of PR #234 into a separate PR.
This PR improves the way we deal with comments and multi-line pragmas in the frontend sanitization steps. Following the previous example, we now also do comment inlining with a one-pass transformer and make all the respective transformers use
inplace=True.The PR also includes a small fix to the previously introduced utility transformers, where we forgot to strip
Noneand empty tuples from the resulting IR. I also needed to adjust the IR graph test - not quite sure why, but it seems safe to me.