FParser: Perform in-place scope attachment during parse #242
Conversation
As variable declarations can be encoutered out of sequence, we have to rebuild/re-attach scopes after parsing the full spec, so that the symbols might be rebuild correctly. This commit adjusts a test to capture this and adds a small custom mapper to do this in place.
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/242/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
==========================================
+ Coverage 92.47% 92.50% +0.02%
==========================================
Files 95 95
Lines 17473 17503 +30
==========================================
+ Hits 16159 16191 +32
+ Misses 1314 1312 -2
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.
Many thanks for grinding through this! I can't claim to fully understand the justification/rationale behind every changed line but it does look sane to me and the expanded testing for edge cases is much appreciated.
Left two comments/suggestions but they are entirely optional.
| name = o.tostr() | ||
| scope = kwargs.get('scope', None) | ||
| parent = kwargs.get('parent') | ||
| if parent: | ||
| scope = parent.scope | ||
| if scope: | ||
| scope = scope.get_symbol_scope(name) |
There was a problem hiding this comment.
I'm assuming this is the discussed handling of derived type members, that ensures
- that we create symbol table entries in the scope where the derived type instance has been declared (and not the typedef scope).
- we attach symbols to the scope where they are declared (and not the closest one)
For future sanity: Would it be worth adding a comment explaining why it's done like this?
| @@ -2929,7 +2979,7 @@ def visit_Assignment_Stmt(self, o, **kwargs): | |||
|
|
|||
| # Special-case: Identify statement functions using our internal symbol table | |||
| symbol_attrs = kwargs['scope'].symbol_attrs | |||
| if isinstance(lhs, sym.Array) and lhs.name in symbol_attrs: | |||
| if isinstance(lhs, sym.Array) and not lhs.parent and lhs.name in symbol_attrs: | |||
There was a problem hiding this comment.
Haha, I think I've hit that one end of last week, too...
| scope = kwargs.get('scope', None) | ||
| parent = self.visit(o.children[0], **kwargs) | ||
| if parent: | ||
| scope = parent.scope | ||
| name = self.visit(o.children[2], **kwargs) | ||
| name = name.clone(name=f'{parent.name}%{name.name}', parent=parent) | ||
| # Hack: Need to force re-evaluation of the type from parent here via `type=None` | ||
| # To fix this, we should stop creating symbols in the enclosing scope | ||
| # when determining the type of drieved type members from their parent. | ||
| name = name.clone(name=f'{parent.name}%{name.name}', parent=parent, scope=scope, type=None) |
There was a problem hiding this comment.
Just looking at the visit_Name, not having actually tested this: Wouldn't passing down parent in the recursion to obtain name work around the scope issue here?
There was a problem hiding this comment.
Alas, no (I tried!). I'm using this hack in two places, each associated with a specific failure in the test base (both in test_scheduler.py). Neither was fixed by passing the parent down (even though I can see what you mean).
I still believe (but not 100% on this), that this is due to us (unintentionally) unrolling all potential local subcomponent variables of a derived-type variable in the local scope. I tried dropping this, but it opened another can of worms, so dropped it. Maybe worth capturing this in an issue instead?
Note: This is the second part to optimising the performance of the FP frontend, as detailed in PR #234. It contributes significantly to the throughput number presented in #234 .
The key idea behind this PR is to avoid the costly rescoping pass of the subroutine/module bodies after the initial IR generation. Instead we now use the
scopeargument passed through the AST-to-IR converter whenever possible.One caveat to this is that we still need to do one
AttachScopespass after parsing thespec, as we might encounter symbols used in shape declarations out of sequence (yay, Fortran!). I have adjusted one of the tests to capture this accordingly.There are also a few little tricks involved, and I've added a small utility to temporarily override dict values, as we now need to be careful when and what to pass down the AST parse tree. There is still one hacky workaround left that forces type re-evaluation from scope for ProcedureDesignators, as the recursive lookup for parents still injects, wrongly-typed pseudo-symbols into the scope (via
Symbol.variable_map). I leave it to a follow-on to fix this correctly.