(Towards #2551) omp barrier removal

[LonelyCat124]

Initial barrier reduction implementation - testing in progress.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (dc66e55) to head (9494ad5).
⚠️ Report is 72 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2967    +/-   ##
========================================
  Coverage   99.90%   99.90%            
========================================
  Files         367      368     +1     
  Lines       51755    51937   +182     
========================================
+ Hits        51705    51887   +182     
  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.

[LonelyCat124]

I think this is ready for a first look @arporter @sergisiso

I tried to add decent comments and documentation (though i think in a couple of places the documentation wording is a bit rough). I've not added anything to the user/dev guide yet - I'll do that once we're close to merging.

I think we should run integration tests with this and nowait enabled again on at least NEMO to check I've not missed anything.

[sergisiso]

@LonelyCat124 I made a first review of this PR. The logic is complex but I it is well encapsulated, commented and tested, so in general I am happy with the implementation, see some minor comments inline.

I've not added anything to the user/dev guide yet - I'll do that once we're close to merging.

The Transformation docstring already documents its use, I wouldn't add more sections if not necessary. What probably would be useful is a complete example (especially if it is not enabled in NEMO proper). Could you do an example similar to examples/nemo/eg1 which takes tra_adv.f90 as input, but applies the nowait and the removal of barriers?

I think we should run integration tests with this and nowait enabled again on at least NEMO to check I've not missed anything.

I agree, I mostly reviewed the implementation, not if the logic itself, I would like to see if it holds for nemo.

[sergisiso]

@LonelyCat124 I brought this to master and triggered the integration tests as I am planning to make some changes to the NEMO action that will conflit with the changes here, so it would be easier if this was merged first.

Also, we have all dependencies installed in the new machine, so the move will be done very soon I hope.

[sergisiso]

Unfortunately the integration tests failed (before reaching the async actions), I re-run the action just in case but get the same results.

[LonelyCat124]

Unfortunately the integration tests failed (before reaching the async actions), I re-run the action just in case but get the same results.

@sergisiso Which integration tests fail? I looked through this PR and behaviour shouldn't change that I can see without the nowaits

[sergisiso]

[LonelyCat124]

I'll try this by hand now and see what happens.

[LonelyCat124]

Looks like this is caused by a file I demoted to be CPU only (sbccflo.f90 or something like that), which has some disabled functions if ASYNC is enabled.

I could add ASYNC_UNSAFE as a set of files? @sergisiso thoughts.

[sergisiso]

@LonelyCat124 Why don't you do:

if ASYNC_PARALLEL:
    OFFLOADING_ISSUES.append("sbccflo.f90")

[LonelyCat124]

Getting NEMO5 to build without an compiler ICE (error 10 or whatever) is annoying, hopefully its fixed on the new machine

[sergisiso]

Unfortunately it's the same in the new machine (and also in Bristol with the grace-hopper). But you can filter zdfsh2.f90 from offloading if needed, as it seems to be causing most BENCH failures.

[LonelyCat124]

@sergisiso This passed integration now after a few runs - if you think this is nearly ready we can avoid having #3075 as a separate PR perhaps.

edit: or wait until this to have that reviewed.

[sergisiso]

@LonelyCat124 This is approved for merging. All comments have been addressed and the tests pass. I have brought the new caching statement here, filtered the zdfsh2.f90, and disabled a couple of the Async tests in order to improve the integration test situation.

Also, the Async result didn't appear the the charts because the name overlap with the previous action, I fixed this so now they should appear.