Skip to content

(Closes #2561) Add support for allocatables in the HoistLocalArraysTrans and check for kind/type dependent symbols#2893

Merged
arporter merged 35 commits into
masterfrom
2561_hoist_allocatables
Aug 5, 2025
Merged

(Closes #2561) Add support for allocatables in the HoistLocalArraysTrans and check for kind/type dependent symbols#2893
arporter merged 35 commits into
masterfrom
2561_hoist_allocatables

Conversation

@sergisiso

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Feb 11, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (9a3e94b) to head (5159673).
⚠️ Report is 36 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2893   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files         371      371           
  Lines       52105    52155   +50     
=======================================
+ Hits        52055    52105   +50     
  Misses         50       50           

☔ 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

sergisiso commented Feb 21, 2025

Copy link
Copy Markdown
Collaborator Author

Integration test passed but the change was not applied almost anywhere because the arrays of interest have:
real(kind=wp), ...
but the transformation validation does:

if isinstance(sym.datatype.intrinsic, DataTypeSymbol):
    continue
if isinstance(sym.datatype.precision, DataSymbol):
    continue

I think we could refine this to check that this symbol is not local to the subroutine and the subroutine doesn't have wilcard imports.

@sergisiso sergisiso self-assigned this Mar 18, 2025
@sergisiso sergisiso added the Blocked An issue/PR that is blocked by one or more issues/PRs. label Jul 3, 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.

Nice work Sergi. I only have minor comments.
I see that examples/nemo/scripts/acc_loops_trans.py has hoisting of local arrays switched on and looking at the 'historical' plot, performance seems OK.

Comment thread src/psyclone/psyir/transformations/hoist_local_arrays_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/hoist_local_arrays_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/hoist_local_arrays_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/hoist_trans.py Outdated
Comment thread src/psyclone/tests/psyir/transformations/hoist_local_arrays_trans_test.py Outdated
Comment thread src/psyclone/tests/psyir/transformations/hoist_local_arrays_trans_test.py Outdated
Comment thread src/psyclone/tests/psyir/transformations/hoist_local_arrays_trans_test.py Outdated
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter This is ready for another look

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

Almost there. Unfortunately last time I missed that it would be better to use reference_accesses() rather than hardwire the check on precision and type symbols. Once that's done, this can go on. Please can you also check why the docs build failed.

Comment thread src/psyclone/psyir/transformations/hoist_local_arrays_trans.py
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter Trying to address your last comment exposed a passthrough bug that we need to solve, so for now I just added a TODO. The linkcheck was failing for password protected links - don't ask me why we didn't fail on these before. Ready for another look.

@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 Sergi, all looks good now.
I'll fire off the integration tests to be on the safe side.

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

Integration tests were all green.
Will proceed to merge.

@arporter arporter merged commit 874c72e into master Aug 5, 2025
12 checks passed
@arporter arporter deleted the 2561_hoist_allocatables branch August 5, 2025 11:41
arporter added a commit that referenced this pull request Aug 5, 2025
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