Skip to content

(closes #3216) Allow specifying explicit private variables for parallel regions#3275

Merged
LonelyCat124 merged 18 commits intomasterfrom
3216_force_private
Jan 22, 2026
Merged

(closes #3216) Allow specifying explicit private variables for parallel regions#3275
LonelyCat124 merged 18 commits intomasterfrom
3216_force_private

Conversation

@sergisiso
Copy link
Copy Markdown
Collaborator

Currently we have Loop.explictly_private_symbols to force a different result than the automatic sharing attributes provide. However it is not possible to do the same with parallel regions. To fix this I moved this attribute to the SharingAttributeMixin. It makes more sense here as it is being set and used by the same class (better encapsulation) and can be used by ALL subclases adding sharing attribute clauses.

There was also the problem that users didn't find out this capability easily. This can be improved by adding a transformation option, as this is the documentation that people is looking at while transforming code.

So when LFRic_atm scripts currently do:

# set some symbols to be PRIVATE
mark_explicit_privates(loop, symbols_to_add)

OMPParallelLoopTrans(omp_schedule="dynamic").apply(
    loop, options=opts
)

can be:

OMPParallelLoopTrans(omp_schedule="dynamic").apply(
    loop, force_private=symbols_to_add, **opts
)

The transformation option logs (but doesn't fail) for symbols not found, to have the same behaviour as mark_explicit_privates.

@sergisiso
Copy link
Copy Markdown
Collaborator Author

sergisiso commented Jan 12, 2026

@MetBenjaminWent may want to be aware of /test these changes

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.95%. Comparing base (152a9c3) to head (3d439ba).
⚠️ Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3275   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         378      378           
  Lines       53783    53803   +20     
=======================================
+ Hits        53761    53781   +20     
  Misses         22       22           

☔ 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.

@sergisiso
Copy link
Copy Markdown
Collaborator Author

@arporter @LonelyCat124 This is ready for a first look

@LonelyCat124
Copy link
Copy Markdown
Collaborator

I'm not going to review this today, just having a quick look at it now - is it worth having an example showing how to use this? We had people in NG-Arch using explicit private on Loop nodes which will no longer function and an example/docs to explain how to use this new functionality on the Directives (which is currently likely to be quite common) might be useful? @sergisiso

@sergisiso
Copy link
Copy Markdown
Collaborator Author

When merging the NG-Arch tickets its use has been standarised to the "mark_explicit_privates" from the LFRic psyclone_tools.py that I show in my top comment, so it is a one-line change. But I agree regarding the example/more docs comment.

@sergisiso
Copy link
Copy Markdown
Collaborator Author

I added and example as suggested (currently in nemo/eg8, but we could move it to the psyir folder once there is no conflict with @LonelyCat124 current changes).

The documentation is now part of the Transformation options. This is done on purpose to make it more discoverable, it is what people will probably open. And it is automatically inherited to all subclasses through kwargs:
image

@LonelyCat124 @arporter This is ready to review again

Copy link
Copy Markdown
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.

@sergisiso This mostly looks ok to me, I've not done all the local stuff yet - couple of copyright dates need updating but I'll do a final review once we can run integration with a local lfric_atm/core

Comment thread examples/nemo/eg8/Makefile Outdated
Comment thread examples/nemo/eg8/add_parallelism.py Outdated
Comment thread src/psyclone/psyir/nodes/loop.py Outdated
Comment thread src/psyclone/psyir/transformations/parallel_loop_trans.py
@sergisiso
Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This is ready for another review, I addressed the comments and also created the associated stfc/lfric_apps PR to pass lfric_atm

Copy link
Copy Markdown
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.

@sergisiso Almost there - I think a result file from eg8 made it into the commits, can you remove that and I'll set integration running.

Comment thread examples/nemo/eg8/output.f90 Outdated
@sergisiso
Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This is ready for another look. I added new Reviewer's steps in the Wiki for changing the lfric hash https://github.com/stfc/PSyclone/wiki/CodeReview/_compare/3483b8f58015f2ae2b4e4f12fde0b1ca45eaa11c

Copy link
Copy Markdown
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.

Running integration one last time for sanity, but ready to merge if all is green.

@LonelyCat124 LonelyCat124 merged commit 08a2e70 into master Jan 22, 2026
15 checks passed
@LonelyCat124 LonelyCat124 deleted the 3216_force_private branch January 22, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants