Additional fixes for IR #55

reuterbal · 2023-03-27T08:09:41Z

Note: This sits on top of #44

I'm filing this separately because it contributes a significant behaviour change for the SubstituteExpressions visitor, which I consider to be sensible but that might require extra testing to make sure it doesn't break behaviour in (untested) edge cases.

In a few places, we ran into infinite recursion in the AttachScopes visitor (although it's not specific to that one, it's just the first to be encountered within the frontends), more specifically, if a declared variable is being used in one of the definining attributes. Encountered examples include:

the initializer expression, e.g. REAL(KIND=JPRB), PARAMETER :: ZEXPLIMIT = LOG(HUGE(ZEXPLIMIT))
the allocation dimensions (which are injected as shape in the Loki IR), e.g.: allocate(levels(jscale)%data(size(levels(jscale-1)%data, 1), size(levels(jscale-1)%data, 2)))
type bound procedure declarations with an explicit interface of the same name: procedure (some_proc), deferred :: some_proc

The cause of the infinite recursion is the fact that we traverse declaration attributes (type.initial, type.shape, type.bind_names) on every symbol use in the LokiIdentityMapper. By adding a recurse_to_declaration_attributes keyword argument to the visitor methods (defaults to True for nodes that are the "authority" on a symbol's type: VariableDeclaration, ProcedureDeclaration, Import), we recurse into these properties only once. Since these are properties that are cached in the symbol table, this is also sufficient (unless transformations rely on the implicit modification of a symbol's declaration when traversing a routine's body, which seems somewhat counterintuitive behaviour).

Conceptually I think this is the right thing to do but the implementation might not be the most elegant...

This also removes quite a bit of traversal overhead in the expression transformer, which shaves off about 10% of the time required to read in cloudsc.F90.

github-actions · 2023-03-27T08:12:53Z

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

codecov-commenter · 2023-03-27T11:33:36Z

Codecov Report

Merging #55 (9cef8f4) into main (c1dd882) will increase coverage by 0.01%.
The diff coverage is 98.48%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   91.77%   91.79%   +0.01%     
==========================================
  Files          86       86              
  Lines       15286    15324      +38     
==========================================
+ Hits        14029    14066      +37     
- Misses       1257     1258       +1

Flag	Coverage Δ
lint_rules	`97.36% <ø> (ø)`
loki	`92.03% <98.48%> (+0.01%)`	⬆️
transformations	`87.02% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
loki/ir.py	`91.88% <ø> (ø)`
loki/visitors/transform.py	`80.26% <ø> (ø)`
loki/expression/mappers.py	`94.56% <97.22%> (+0.01%)`	⬆️
loki/expression/expr_visitors.py	`99.32% <100.00%> (+0.06%)`	⬆️
loki/frontend/fparser.py	`92.99% <100.00%> (+0.02%)`	⬆️
loki/frontend/ofp.py	`97.52% <100.00%> (ø)`
loki/frontend/util.py	`93.02% <100.00%> (ø)`
loki/transform/transform_associates.py	`100.00% <100.00%> (ø)`

reuterbal · 2023-04-12T17:40:23Z

I had to update this to account for a specific fparser behaviour (see stfc/fparser#400) which has quite significant impact and for which we may have to watch out.

mlange05

Ok, first of all, many thanks for figuring this stuff out. I will certainly appreciate 10% of CLOUDSC parsing time reduction.

I think the behavioural change is fine, but the implementation that exposes details of the outer IR to the inner is not ok, as it goes against the existing visitor pattern and will be hard to remove at a later stage.

In addition, it might be useful to separate the frontend change relating to fparser from the SubstituteExpression change to streamline the merge process a little.

mlange05 · 2023-05-03T08:47:42Z

loki/expression/mappers.py

@@ -520,6 +521,10 @@ def __init__(self, invalidate_source=True):
    def __call__(self, expr, *args, **kwargs):
        if expr is None:
            return None
+        kwargs.setdefault(
+            'recurse_to_declaration_attributes',
+            'current_node' not in kwargs or isinstance(kwargs['current_node'], DECLARATION_NODES)


😬 This sort-of starts violating all sorts of encapsulation rules, as the expression mapper (inner IR) is now aware of (and depends on) the implementation details of the outer IR. This creates even more cyclic dependencies that become hard to unfuddle later.

Instead, I can see two way to do this more neatly here:

Honour kwargs["recurse_to_declaration_attributes"] and set it in SubstituteExpression.visit_Declaration(...). This results in only a single style of behviour, where declarations only recurse into the type, which is certainly what we want as default.

Otherwise, or in addition to the above, we could set a constructor flag in both SubstituteExpression and the Mapper to always_recurse_into_types (or similar), as a hard override to force the current behaviour if the user wants/needs this.

Since this here is constrained purely to substitutions from the same map, I think we should be safe with only the first case, and no universal override. For general "for all variable" traversals, this might not hold true, but for map-based substitutions, I cannot see an edge-case where we'd need this (but I could be wrong here).

Yes, good point, thanks for pointing that out! I'll try your first proposal, I think it is essentially what I was shooting for but with a much more awkward solution, and I don't remember why I picked that route (it's been so long 😏)

Yes, looks good now. Many thanks for addressing this.

mlange05 · 2023-05-03T08:50:45Z

loki/expression/mappers.py

-            expr.scope.symbol_attrs[expr.name] = expr.type.clone(bind_names=as_tuple(bind_names))
+        if kwargs['recurse_to_declaration_attributes']:
+            _kwargs = kwargs.copy()
+            _kwargs['recurse_to_declaration_attributes'] = False


I'm not entirely sure I understand why the bind_names are so special here that they require this entire code block. Maybe a comment why simple calling self.rec(...) is not sufficient here might make this a little more accessible?

Good point. I have tried to restructure the control flow in a way that makes it clearer what's going on, and add comments.
In essence: only when recurse_to_declaration_attributes is True, we recurse to all the bits that are stored in a symbol's type. But, because Fortran, we don't want this recursion to happen while actually updating these declaration attributes. Reason is that the declared variable can actually appear in its own initializer expression:

REAL var = HUGE(var)

Or "it" can appear in an allocation:

TYPE(SOME_TYPE), ALLOCATABLE :: var(:) ... DO j=2,n ALLOCATE(var(j)%field(size(var(j-1)%field))) END DO

And since we insert the dimensions from allocate statements as type.shape, this now becomes also part of the symbol table entries...

Ahhhh, thanks for the explanation. Makes sense and looks much clearer now.

…aration

…before dispatching to expr tree mappers

mlange05

Thanks for addressing and fixing this all. This is now GTG, and going in next.

I've confirmed with EC-physics regression and indeed we're shaving 15-20% off certain stages in the pipeline with this change, so again, many, many thanks! 🙏 🙏 🙏

reuterbal force-pushed the nabr-linter-fixes branch 2 times, most recently from adb7d8e to 1e4d3c1 Compare March 27, 2023 10:18

reuterbal requested review from mlange05 March 27, 2023 12:53

reuterbal marked this pull request as ready for review March 27, 2023 12:53

reuterbal force-pushed the nabr-linter-fixes branch 2 times, most recently from 0576845 to 0bb0a7b Compare March 31, 2023 19:06

mlange05 requested changes May 3, 2023

View reviewed changes

reuterbal force-pushed the nabr-linter-fixes branch from 29a5a80 to f01ea67 Compare May 3, 2023 10:51

reuterbal added 10 commits May 4, 2023 12:16

Fix infinite recursion for declarations with the symbol in initializer

4b22bb9

Do not invalidate source in statement function injection

16cd442

Store source for helper var declaration representing return type

54462c0

Recurse into declaration attributes only from Import and VariableDecl…

47ed3a2

…aration

Retain source object for injected statement functions

766c201

Fix infinite recursion on abstract procedure declaration

86be4d5

Workaround for fparser behaviour

3b6f00d

Set recurse_to_declaration_attributes in the control flow visitors …

64d25b5

…before dispatching to expr tree mappers

Make control flow in map_variable_symbol cleaner

0ee8301

Eliminate another stray current_node argument

9cef8f4

reuterbal force-pushed the nabr-linter-fixes branch from eaaebaf to 9cef8f4 Compare May 4, 2023 11:16

mlange05 approved these changes May 4, 2023

View reviewed changes

mlange05 merged commit 14c6c82 into main May 4, 2023

mlange05 deleted the nabr-linter-fixes branch May 4, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional fixes for IR #55

Additional fixes for IR #55

reuterbal commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

codecov-commenter commented Mar 27, 2023 •

edited

Loading

reuterbal commented Apr 12, 2023

mlange05 left a comment

mlange05 May 3, 2023

reuterbal May 3, 2023

mlange05 May 4, 2023

mlange05 May 3, 2023

reuterbal May 3, 2023

mlange05 May 4, 2023

mlange05 left a comment

Additional fixes for IR #55

Additional fixes for IR #55

Conversation

reuterbal commented Mar 27, 2023 • edited Loading

github-actions bot commented Mar 27, 2023 • edited Loading

codecov-commenter commented Mar 27, 2023 • edited Loading

Codecov Report

reuterbal commented Apr 12, 2023

mlange05 left a comment

Choose a reason for hiding this comment

mlange05 May 3, 2023

Choose a reason for hiding this comment

reuterbal May 3, 2023

Choose a reason for hiding this comment

mlange05 May 4, 2023

Choose a reason for hiding this comment

mlange05 May 3, 2023

Choose a reason for hiding this comment

reuterbal May 3, 2023

Choose a reason for hiding this comment

mlange05 May 4, 2023

Choose a reason for hiding this comment

mlange05 left a comment

Choose a reason for hiding this comment

reuterbal commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

codecov-commenter commented Mar 27, 2023 •

edited

Loading