Skip to content

(Towards/Close #3315, Closes #3590) New implementation of lfric loop fuse trans - #3595

Open
LonelyCat124 wants to merge 14 commits into
masterfrom
3315_lfric_loop_fusion
Open

LonelyCat124 wants to merge 14 commits into
masterfrom
3315_lfric_loop_fusion

Conversation

@LonelyCat124

@LonelyCat124 LonelyCat124 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Initial implementation for the new lfric loop fuse trans setup.
There are still for sure some things to sort out but it seems to work for our tests and solves ANY_SPACE things.

I'd like to have an investigate of being able to fuse more than 2 LFRicLoops, provided they all meet some criteria (i.e. some_space is provided, they're all on the same field etc., or that they all have the same space at runtime) but I'd need to know if that is useful @christophermaynard @MetBenjaminWent ? Its probably a more substantial implementation but I think its possible.

@sergisiso

Copy link
Copy Markdown
Collaborator

Here is the example of min and max builtins that could be easy targets to fuse: src/psyclone/tests/test_files/lfric/15.10.9_min_max_X_builtin.f90

@LonelyCat124 LonelyCat124 changed the title (Towards/Close #3315) New implementation of lfric loop fuse trans (Towards/Close #3315, Closes #3590) New implementation of lfric loop fuse trans Sep 18, 2026
@arporter

Copy link
Copy Markdown
Member

Some notes from our meeting.
I think the check list for whether or not two kernels can be fused is:

  • Do the two kernels OPERATE_ON the same thing (dofs or cell columns)?
  • Is their iteration space determined by the same field/operator?
  • Or, do their iteration spaces correspond to the same function space (static check, will need to resolve 'ANY_SPACE' to an actual space)?
  • [Optionally] do the fuse but protect it with a runtime check on the function spaces.

We think most of the the benefit will come from fusing built-ins (particularly those that do MIN/MAX on the same field).
For user-provided kernels that operate on cell-columns, I think they can be fused following the rules above, provided that the DA doesn't determine that a halo-exchange is required between them. (This allows for e.g. a kernel that increments a field on W2 that is also written to in the first kernel and also for stencil accesses.) @christophermaynard thinks the opportunities for doing this kind of fusion will be very limited.

We also discussed 'constant propagation' but realised that often, a setval_c is used to initialise a field that is then incremented by a kernel that loops over cell-columns. Since, for continuous function spaces, different cell-columns will increment the same dof it will not be possible to do 'constant propagation' in this case.

@LonelyCat124

Copy link
Copy Markdown
Collaborator Author

I think the new implementation does most of this, what I couldn't work out is why is this necessary:

Is their iteration space determined by the same field/operator?

This can be either:

  1. to save searching for what the space is if they're both built ins?
  2. If we have 2 any_space Kerns without a non-any_space kern on the field to be able to work out the iteration space?

Does that make sense @arporter ?

I am also unsure I've done anything re: " I think they can be fused following the rules above, provided that the DA doesn't determine that a halo-exchange is required between them". I'll have to work that out next.

@arporter

Copy link
Copy Markdown
Member

This can be either:

  1. to save searching for what the space is if they're both built ins?
  2. If we have 2 any_space Kerns without a non-any_space kern on the field to be able to work out the iteration space?

Yes, sorry, I was a bit slack in how I phrased it. If the iteration space is defined by the same field/operator then we don't have to look at function spaces. We probably still need to worry about stencil accesses. In fact, now that I write that, we need to be careful about kernels that update more than one argument (they are permitted to do this). A first step might be to refuse to fuse such cases.

@LonelyCat124

Copy link
Copy Markdown
Collaborator Author

I think I've added a check in validate for writing to only one field (I think).
I think it would be good to have more things in validate than apply, however there are challenges due to the different behaviour with conditional_fusion, which we need a lot of information to determine whether its required or not.

@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.23404% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.96%. Comparing base (1832b6f) to head (ab9bed4).
⚠️ Report is 161 commits behind head on master.

Files with missing lines Patch % Lines
...ain/lfric/transformations/lfric_loop_fuse_trans.py 84.61% 18 Missing ⚠️

❌ Your project check has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files
@@             Coverage Diff             @@
##            master    #3595      +/-   ##
===========================================
- Coverage   100.00%   99.96%   -0.04%     
===========================================
  Files          399      403       +4     
  Lines        56467    56905     +438     
===========================================
+ Hits         56467    56887     +420     
- Misses           0       18      +18     

☔ View full report in Codecov by Harness.
📢 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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants