Skip to content

(closes #3052) Fix missing psylayer openmp symbols#3053

Merged
arporter merged 7 commits into
masterfrom
fix_missing_th_idx_symbols
Jul 21, 2025
Merged

(closes #3052) Fix missing psylayer openmp symbols#3053
arporter merged 7 commits into
masterfrom
fix_missing_th_idx_symbols

Conversation

@sergisiso

Copy link
Copy Markdown
Collaborator

EPCC reported that combining everything mentioned in the ticket the "th_idx" and "omp_get_thread_num" declarations are missing. I added a similar test.

Both have in common that are declared on self. scope, if I move their declaration to self.ancestor(Routine).scope the problem is gone. EPCC has verified that this works for them. Other symbols declared on self.scope are fine.

@sergisiso sergisiso requested a review from arporter July 11, 2025 14:18
@sergisiso sergisiso self-assigned this Jul 11, 2025
@codecov

codecov Bot commented Jul 11, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (c16ab26) to head (d19ad0d).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3053   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files         366      366           
  Lines       51677    51678    +1     
=======================================
+ Hits        51637    51638    +1     
  Misses         40       40           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arporter arporter added LFRic Issue relates to the LFRic domain NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH under review and removed ready for review labels Jul 14, 2025

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this Sergi. It would be good to take a bit of time here to figure out what is actually going wrong (or at least, as much detail as you can provide) so that we can fix it properly in slower time. It would also be good to add compilation testing to the new test if possible.
I'll launch the integration tests in the meantime (unless you have already done so).

Comment thread src/psyclone/psyGen.py
Comment thread src/psyclone/tests/psyir/transformations/profile_test.py
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter This is ready for another review

@sergisiso

Copy link
Copy Markdown
Collaborator Author

Jaffery confirmed that the new fix still works for EPCC profiling

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well done on tracking this down.
I've just set the integration tests going again as this is an important change (although I'm not sure how much of psy_data_node they touch really). If they are OK then this is good to go.

@sergisiso

Copy link
Copy Markdown
Collaborator Author

There is a nemov5 file that failed, but it is not related to this PR (has not psy_data or lfric), do you want me to attempt filtering it here or proceed and deal with it in a NEMO PR?

@arporter

Copy link
Copy Markdown
Member

I'll merge this one and then we can fix it separately I think.

@arporter arporter merged commit a9f77e2 into master Jul 21, 2025
12 checks passed
@arporter arporter deleted the fix_missing_th_idx_symbols branch July 21, 2025 08:53
arporter added a commit that referenced this pull request Jul 21, 2025
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 NG-ARCH Issues relevant to the GPU parallelisation of LFRic and other models expected to be used in NG-ARCH under review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants