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

Correct symbols after derived type enrichment #450

Merged
merged 8 commits into from
Nov 29, 2024

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Nov 25, 2024

Currently sits on top of PR #448 , so needs rebase once that is merged.

This PR aims to make the state of subroutine/modules after enrichment with derived type info to the a priori provision of type info via definitions. The key difference here is that, after type info has been attached to imports and symbol tables, the expression trees need to be rebuild, so that DeferredTypeSymbol.clone() yields Array or Scalar variable symbols.

For this purpose, I've introduced a base class ExpressionTransformer that takes only the basic expression re-build mechanism from SubstituteExpressions and exposes this generically via a static expr_mapper attribute (similar to the ExpressionFinder base class. This defaults to LokiIdentityMapper, which is enough to rebuild expressions.

Along the way to getting this un-muddled in my head, I also got annoyed by derived-type symbols in Import nodes being typed as ProcedureSymbols, so I implemented a mechanically equivalent DerivedTypeSymbol and added a test that uses it to the frontends, to explicitly test the use of type info viadefinitions to create the right symbol types.

Tow other piggy-backed housekeeping changes:

  • Bug fix for FP frontend, where we now add the return-type into the symbol-attrs before recursing on the body, so that any use of the return symbol is scoped correctly.
  • Split frontend tests into general frontend test and regex tests (the latter is much longer 😮 )
  • Cleaned up the imports at the top of subroutine tests

@mlange05 mlange05 requested a review from reuterbal November 25, 2024 09:40
Copy link

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

@mlange05 mlange05 force-pushed the naml-enriched-derived-type-symbols branch 2 times, most recently from 41b37f6 to 9fcbb07 Compare November 25, 2024 12:44
This is implicitly used by `SubstatituteExpressions`, but can also
run a simple `LokiIdentityMapper` (the default) over any expression.
After the type information has been correctly attached, we can rebuild
the epxression trees to replace `DeferredTypeSymbols` with `Scalar`
and `Array` symbols.
@mlange05 mlange05 force-pushed the naml-enriched-derived-type-symbols branch 2 times, most recently from 605166f to 8a975bf Compare November 26, 2024 15:05
We previously (ab)used `ProcedureSymbol` objects for this. The new
symbol type also uses the exact same mechanics as procedure symbols
for interfacing twith pymbolic types.
@mlange05 mlange05 force-pushed the naml-enriched-derived-type-symbols branch from 8a975bf to 3aaef08 Compare November 27, 2024 04:14
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.28%. Comparing base (c3aaa57) to head (fa42d02).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   93.24%   93.28%   +0.03%     
==========================================
  Files         220      220              
  Lines       41137    41202      +65     
==========================================
+ Hits        38360    38436      +76     
+ Misses       2777     2766      -11     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 93.24% <100.00%> (+0.03%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mlange05 mlange05 marked this pull request as ready for review November 27, 2024 04:57
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.

Many thanks, this misclassification of imported type names was a bit of a sore point...

Implementation looks good to me. Out of curiosity, how are constructor calls represented now? For example, something like

use some_mod, only: type_name

...
type(type_name) :: obj
...
obj = type_name(arg1, arg2)

Is this an inline call with DerivedTypeSymbol('type_name')?

Comment on lines +1189 to 1192
assert isinstance(v, sym.ProcedureSymbol)
assert isinstance(v.type.dtype, ProcedureType)
assert v.type.external is True
assert v.type.dtype.procedure == BasicType.DEFERRED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Codecov thinks these lines are never executed, so it looks to me like the test isn't quite doing what it's meant to do.
This isn't related to this PR but maybe you can have a quick look if you don't mind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, good spot for codecov! These lines were indeed implicitly skipped, as the test was attempting to check ProcedureDeclaration nodes, but was iterating VariableDeclaration, likely as the former had been introduced much later than the test. I've pushed a fix that should rectify that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"mumble, grumble, ... the system works(!), ... grumble"

@mlange05
Copy link
Collaborator Author

Yes, the above example will become an InlineCall with a DerivedTypeSymbol as call.function. At some point I tried to fix this, but had to revert this effort. The problem is that in the symbol table of the enclosing function we would have to store a ProcedureType a DerivedType under the same key - or if we don't, risk any arbitrary clone of the call.function symbol changing the type of the symbol according to what is currently stored in the symbol_attrs.

This re-enables the actual check on the declaration symbols.
@reuterbal reuterbal merged commit ba7e230 into main Nov 29, 2024
13 checks passed
@reuterbal reuterbal deleted the naml-enriched-derived-type-symbols branch November 29, 2024 14:31
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.

2 participants