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

Fix bugs in handling of Fortran types (closes #2237) #2255

Merged
merged 36 commits into from
Oct 2, 2023

Conversation

rupertford
Copy link
Collaborator

When parsing Fortran types (structures) the extends clause can be discarded and initial values provided for entries in the type can also be discarded. This PR fixes these two problems.

@rupertford rupertford added in progress LFRic Issue relates to the LFRic domain PSyIR Core PSyIR functionality labels Aug 10, 2023
@rupertford rupertford self-assigned this Aug 10, 2023
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (12c865f) 99.84% compared to head (915df19) 99.84%.
Report is 1 commits behind head on master.

❗ Current head 915df19 differs from pull request most recent head e2ec8e1. Consider uploading reports for the commit e2ec8e1 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2255   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         344      343    -1     
  Lines       45931    46011   +80     
=======================================
+ Hits        45860    45940   +80     
  Misses         71       71           
Files Coverage Δ
...mmon/transformations/kernel_module_inline_trans.py 100.00% <ø> (ø)
src/psyclone/dynamo0p3.py 99.81% <100.00%> (-0.01%) ⬇️
src/psyclone/psyir/backend/fortran.py 100.00% <100.00%> (ø)
src/psyclone/psyir/frontend/fortran.py 100.00% <100.00%> (ø)
src/psyclone/psyir/frontend/fparser2.py 99.86% <100.00%> (-0.01%) ⬇️
src/psyclone/psyir/nodes/loop.py 100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/datatypes.py 100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/symbol_table.py 100.00% <100.00%> (ø)
src/psyclone/tests/dynamo0p3_test.py 100.00% <ø> (ø)
...lone/tests/psyir/backend/fortran_gen_decls_test.py 100.00% <ø> (ø)
... and 11 more

... and 7 files with indirect coverage changes

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

@rupertford
Copy link
Collaborator Author

Ready for first review from @arporter or @sergisiso

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@rupertford I made just minor suggestions and I think this potentially fixes (represents as UnknownFortranType) some abstract types in Numa3d and Nemo.

There is a subtle issue when we have code that looks for all the symbols "used" in a region, e.g. the two inline transformations
(src/psyclone/domain/common/transformations/kernel_module_inline_trans.py line 184) and (src/psyclone/psyir/transformations/inline_trans.py line 678) and maybe others.
As the initialisation expression could have symbols that are not declared in the same symbol table and these methods will miss to check if the symbol can be safely inlined.
However, these ad-hoc implementations are already very error-prone and I would like to centralise this logic in a single place (maybe a symbol_table utility or the self.reference_accesses itself). So I will create a separate issue for it.

src/psyclone/psyir/backend/fortran.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/datatypes.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/datatypes.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/datatypes.py Outdated Show resolved Hide resolved
@rupertford
Copy link
Collaborator Author

Ready for next review from @sergisiso, but this PR is to check that the path I have taken is the right one before I fix tests and tidy up.

@sergisiso
Copy link
Collaborator

Unfortunately this still makes the NEMO for GPU ~10% slower,
Master:
image
This PR:
image

The diff with master in zdftmx.f90:

<       !$omp target
<       !$omp loop collapse(2)
<       do idx_26 = loop_start_26, loop_stop_26, 1
<         do idx_27 = loop_start_27, loop_stop_27, 1
<           zkz_1(idx_27,idx_26) = zkz_1(idx_27,idx_26) + e3w_n(idx_27,idx_26,jk) * MAX(0.e0, rn2(idx_27,idx_26,jk)) * rau0 * &
< &zavt_itf(idx_27,idx_26,jk) * tmask(idx_27,idx_26,jk) * tmask(idx_27,idx_26,jk - 1)
<         enddo
<       enddo
<       !$omp end loop
<       !$omp end target
---
>       zkz_1(:,:) = zkz_1(:,:) + e3w_n(:,:,jk) * MAX(0.e0, rn2(:,:,jk)) * rau0 * zavt_itf(:,:,jk) * tmask(:,:,jk) * tmask(:,:,jk - 1)

This is because this files has 2 subroutines what have rau0, and it gets resolved to a DataSymbol in the first one (zdf_tmx) but in the second one (tmx_itf) it is still a generic Symbol. Note that rau0 is a module symbol and should be represented by the same symbol object, but they are 2 different objects.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@rupertford Unfortunately the symbol resolution for NEMO is not quite working yet, but we are getting closer. Can you look at the following issues.

(I am fine with leaving the debug_string for the inlining as it is, and discuss it in its own issue)

src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/symbol_table.py Outdated Show resolved Hide resolved
@rupertford
Copy link
Collaborator Author

Ah the PR that keeps giving. I'll take a look at the problems.

@rupertford
Copy link
Collaborator Author

Ready for next review from @sergisiso

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@rupertford Thanks for addressing all comments. This PR is now passing the integration tests without degrading the performance and all remaining discussions are addressed by other issues marked as TODOs, so it can proceed to be merge.

I fixed small remaining pycodestyle, but all other tests pass, patch codecov is complete and the docs build without new issues.

@sergisiso
Copy link
Collaborator

I will wait for #2269 to be merged first

@sergisiso sergisiso temporarily deployed to integration October 2, 2023 09:05 — with GitHub Actions Inactive
@sergisiso sergisiso merged commit 48f9f26 into master Oct 2, 2023
10 checks passed
@sergisiso sergisiso deleted the 2237_structuretype_initial_values branch October 2, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LFRic Issue relates to the LFRic domain PSyIR Core PSyIR functionality ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants