stfc / PSyclone

Domain-specific compiler and code transformation system for Finite Difference/Volume/Element Earth-system models in Fortran
BSD 3-Clause "New" or "Revised" License
104 stars 27 forks source link

257 verify loop fusion #2707

Open hiker opened 3 weeks ago

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.86%. Comparing base (70f9793) to head (1a275a4).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2707 +/- ## ======================================= Coverage 99.86% 99.86% ======================================= Files 354 354 Lines 49082 49137 +55 ======================================= + Hits 49018 49073 +55 Misses 64 64 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hiker commented 3 weeks ago

I've found an additional problem with the loop fuse: the loops can be specified in the wrong order, and this will be accepted by validate. But during fuse the loop bodies will be ending up in the wrong order!

hiker commented 3 weeks ago

Ready for a first review. I have moved the dependency analysis required in the validation function into the DependencyTool , anticipating the need to use the existing code for DA. Unfortunately (and as discussed), I don't have time atm to implement the full DA for fusion, I only verify that all accesses in the fused dimension are consistent (i.e. no stencil read access for variables that are also written) - this can result in some cases to reject fusion when it is still valid. But we have a 'force' option so we can overwrite this (and much better to reject a bit too keen compared with the current state where we incorrectly fuse).

Note that in order to support renaming of loop variables I had to add support to SymPy to indicate that some variables have the same value.

I also moved most of the failing loop fusion tests into the dependency tools test (to reach 100% coverage of the latter).

I've triggered the CI as well.

hiker commented 1 week ago

Ready for next review. I've triggered the CI, since I'd guess there is a good chance for this change affecting some transformations.