Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statement function parsing on-the-fly #132

Merged
merged 5 commits into from
Sep 5, 2023
Merged

Conversation

mlange05
Copy link
Collaborator

Previously, statement functions (on of the true beauties of the Fortran standard!) have been retro-fitted to the IR after an initial parse, since the FParser frontend cannot distinguish them from regular array accesses without type information. As pointer out in issue #104, the Loki FP frontend can also fall into the trap of getting this wrong by preprending the last stmt function to the subroutine body instead of appending it to the spec.

This PR now fixes the statement function detection by using our internal symbol table to detect them on-the-fly and thus construct them as part of the original parse. This avoids some very(!) costly IR transformations (rebuilds) that we otherwise perform routinely. Since the StmtFunc objects are already in the partial IRs for spec and body, re-shuffling the mismatch between body and spec is not much easier and can be done before creating the final Subroutine object.

This PR now does:

  • Detect StmtFuncs in FP frontend by looking for explicitly defined scalars that are used with notation on the LHS of an assignment
  • Before finalising the Subroutine, look for stray StmtFuncs at the beginning of the body
  • Update the original Scalar declaration symbol after the subroutine is finalized; this is needed as the NodeLazyLookup requires all declarations to be attached to the spec
  • Adjust the existing StmtFunction test to capture the corner-case in issue Start of computation misinterpreted when mixing multiple: variable and statement function declarations. #104.
  • Remove the now obsolete retro-fitting utility

The PR seems to drop the full-parsing time of CLOUDSC (dwarf version) by about 25%, which is a nice side-effect, and has been verified with EC-physics regression.

One final note (@reuterbal), it might make sense to create a dedicated Procedure => Subroutine | Function | StmtFunctionset of classes, with Procedure an ACB, to fully capture the specific procedure types. This would then allow the explicit "nesting" of StmtFuncs in the IR without the use of theLazyNodeLookup`, as it would create a StmtFunc as a scope in its own right. But that's just meant as food for thought. 😉

@github-actions
Copy link

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

This happens to have triggered but seems actually wrong: If we
redeclare the argument in the member and use it as an array, it
cannot be declared scalar.
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #132 (a7b778f) into main (cc5f016) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
- Coverage   92.11%   92.10%   -0.02%     
==========================================
  Files          90       90              
  Lines       16630    16586      -44     
==========================================
- Hits        15319    15276      -43     
+ Misses       1311     1310       -1     
Flag Coverage Δ
lint_rules 96.01% <ø> (ø)
loki 92.02% <100.00%> (-0.02%) ⬇️
transformations 91.78% <ø> (ø)

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

Files Changed Coverage Δ
loki/frontend/fparser.py 93.07% <100.00%> (+0.09%) ⬆️
loki/frontend/util.py 90.82% <100.00%> (-2.20%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done and a much better solution than the post-hoc fiddling we did previously. That this has to be done only for fparser is due to the fact that OMNI inlines them anyway and OFP doesn't support them in the first place?

@reuterbal reuterbal merged commit a987012 into main Sep 5, 2023
11 checks passed
@reuterbal reuterbal deleted the naml-stmt-func-fixes branch September 5, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start of computation misinterpreted when mixing multiple: variable and statement function declarations.
2 participants