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

(Closes #2845) fix for inline symbol bug #2848

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

arporter
Copy link
Member

No description provided.

@arporter arporter added in progress NEMO Issue relates to the NEMO domain NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH labels Jan 14, 2025
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (2307ac7) to head (dbce296).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2848   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         359      359           
  Lines       50833    50833           
=======================================
  Hits        50777    50777           
  Misses         56       56           

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

@arporter
Copy link
Member Author

A small change to the inlining transformation so that symbols are added to the table of the Routine of the call site rather than to the table of the local scope. This allows us to spot problems in the validate rather than crashing at the end of the appy() method. The integration tests will need to be run but I don't want to do that during the day while Glados is busy.

LonelyCat124
LonelyCat124 previously approved these changes Jan 15, 2025
Copy link
Collaborator

@LonelyCat124 LonelyCat124 left a comment

Choose a reason for hiding this comment

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

Need to check if the integration tests are ok, but the code changes all look fine, and tests/coverage/etc. are all fine.

Edit: Someone started the integration tests about 30 minutes ago - @arporter is this ok or do we need to stop them and try them later?

@arporter
Copy link
Member Author

It's OK, it was me and I see NEMO v.4 failed :-(

@arporter
Copy link
Member Author

Failure was:

  File "/home/gh_runner/actions-runner/_work/PSyclone-mirror/PSyclone-mirror/.runner_venv/lib/python3.13/site-packages/psyclone/psyir/symbols/symbol_table.py", line 1793, in rename_symbol
    raise ValueError(
        f"The symbol argument of rename_symbol() must belong to this "
        f"symbol_table instance, but '{symbol}' does not.")
ValueError: The symbol argument of rename_symbol() must belong to this symbol_table instance, but 'psyclone_cmp_int: DataSymbol<Scalar<BOOLEAN, UNDEFINED>, Automatic>' does not.
make: *** [Makefile:70: psycloned-openacc_kernels/dynldf.f90] Error 1

and must be because we are adding symbols to tables in nested scopes rather than the parent Routine scope.

@LonelyCat124
Copy link
Collaborator

Ok - I'll sent it back to you to resolve then

@LonelyCat124 LonelyCat124 self-requested a review January 15, 2025 12:26
@LonelyCat124 LonelyCat124 dismissed their stale review January 15, 2025 12:30

Integration tests failed

@arporter
Copy link
Member Author

That was easier than I expected. Will wait until tonight to trigger integration tests again.

@arporter
Copy link
Member Author

NEMO4 OpenACC kernels integration test failed again :-( Will investigate.

@arporter
Copy link
Member Author

The failure is:

nemo4_src]$ psyclone -I ./ -s ~/PSyclone/examples/nemo/scripts/acc_kernels_trans.py nemogcm.f90

It is the inlining of nemo_init that causes the failure with a symbol that is unresolved in both the routine and, at the point of inlining, the call site:

psyclone.psyir.symbols.symbol.SymbolError: PSyclone SymbolTable error: A symbol named 'sn_ctl' is present but unresolved in one or both tables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress NEMO Issue relates to the NEMO domain NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants