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

(closes #1370) Adds 'collapse' and 'ignore_dependencies_for' options to parallel_loop_trans #2660

Closed sergisiso closed 2 months ago

sergisiso commented 2 months ago

Note that I changed the definition of the collapse option when it is an integer to mean collapse "upto x loops". Before it was collapse exactly 'x' loops or fail. But I found it hard to use for programmatical changing complex code as loops comes in all shapes. It could also be "atleast x loops" or fail.

Regardless, in practice in NEMO we always use the boolean option which means do as much as possible.

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 99.86%. Comparing base (3d441ad) to head (36d521d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2660 +/- ## ======================================= Coverage 99.86% 99.86% ======================================= Files 352 352 Lines 48650 48706 +56 ======================================= + Hits 48585 48641 +56 Misses 65 65 ```

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

sergisiso commented 2 months ago

@arporter This is ready for a first review.

Note that the NEMO openmp for CPU is slower, Glados had several users at the time that may explain the behaviour, but I also removed some "forced" loop parallelisations that may impact the performance (I will repeat the integration test when its idle and if the slowdown is still that big, I will take a more nuanced approach).

Also you may want to check if the changes to ACCParallelTrans have any impact to the LFRic GPU scripts that are not in the repository.

sergisiso commented 2 months ago

@arporter This is ready for another look

arporter commented 2 months ago

Integration was green.