Move some parts of btstep() into subroutines

[theresa-cordero]

Modularize btstep

(edited to correct the names of the new routines)

This PR moves some parts of btstep() into subroutines:

• btstep_ubt_from_layer
• btstep_find_Cor
• truncate_velocities
• btloop_eta_predictor
• store_u_for_OBCs and store_v_for_OBCs
• btloop_update_v
• btloop_update_u
• btstep_layer_accel

This eliminates the duplication of the u and v update code in the bt loop, but preserves the BT_USE_OLD_CORIOLIS_BRACKET_BUG in the update when v is done second.

The creation of these subroutines should not change answers. I have tested it in the Baltic OM4 0.25 case with BT_USE_OLD_CORIOLIS_BRACKET_BUG both True and False.

Each routine has many arguments and I have tried to correctly label them intent(in) or intent(inout) as appropriate. Some variables did not have clear comments describing them in this module (ex. use_BT_cont or jsvf), so I have not put doxygen comments describing them in this commit. These can be added when I address the review.

The short description of each routine is:

btstep_ubt_from_layer: Calculate the zonal and meridional velocity from the 3-D velocity.
btstep_find_Cor: Find the Coriolis force terms _zon and _mer.
truncate_velocities: Do a CFL-based truncation of any excessively large batotropic velocities.
btloop_eta_predictor: A routine to set eta_pred and the running time integral of uhbt and vhbt.
store_[uv]_for_OBCs: Store the existing velocities and transports for use with open boundary conditions.
btloop_update_v: Update meridional velocity.
btloop_update_u: Update zonal velocity.
btstep_layer_accel: Calculate the zonal and meridional acceleration of each layer due to the barotropic calculation.

[Hallberg-NOAA]

Thank you for this contribution, @theresa-cordero. There are a number of issues here with the openMP directives and some doxygen comments that need to be addressed, and some of the new routines strike me as not being quite the right granularity. I have put together a branch of MOM6 at https://github.com/Hallberg-NOAA/MOM6/tree/modularize_btstep_rebase in which I am attempting to work through these issues, and which might be the basis for some modifications to this pull request.

[theresa-cordero]

Thanks Bob for the feedback and the changes on you branch. I have decided to merge in your branch and made one more additional change by renaming btloop_setup_eta to btloop_eta_predictor.

[herrwang0]

This is more of a question than a comment.

I really like the idea of using btloop_update_[uv] to remove duplicate code. The rest of the new subroutines seem to be used only once. I agree grouping them together does make subroutine btstep more readable. But I wonder if we need to consider the overhead of calling these single-used subroutines (and the argument lists are quite chunky too)? Or the benefit of a clearer structure outweighs the overhead?

[Hallberg-NOAA]

Thank you for these changes refactoring the barotropic solver, which should make the it easier to understand.

In addition to closely inspecting these changes, to verify the correctness of this refactoring, I have re-implemented all of these changes myself (e.g., copying the previous working parts of the code into the new routines) and semi-independently come up with the same results as Theresa did. I am therefore confident that all of these changes are correct.

[yichengt900]

I can confirm that this refactor has passed all existing regression tests for the CEFI regional configurations.

[marshallward]

But I wonder if we need to consider the overhead of calling these single-used subroutines (and the argument lists are quite chunky too)? Or the benefit of a clearer structure outweighs the overhead?

I tested this PR in double_gyre with ifort and -O3 optimization, and only btstep_layer_accel appears to be inlined. The other functions are being called normally and the stack pointer code is rather large, but it doesn't necessarily mean a lot of overhead.

The double_gyre output is a little too simple to trust, but it does not appear to be much overhead; the function is maybe 1-2% extra overhead than leaving the loops in-place.

[Hallberg-NOAA]

In this case, I do not think that the added subroutines are likely to be a significant added computational burden. These routines are within the same file, so they should be good candidates for inlining. In addition, this PR is intended to be a preliminary step towards more extensive modifications that may add new capabilities (e.g., fusing the sea-ice with the ocean barotropic solver) or running on new platforms (e.g,. GPUs).

(I would trust Marshall's comment more than mine regarding the inlining; I was writing and hit send before seeing that Marshall had also responded.)

[herrwang0]

@marshallward @Hallberg-NOAA Thanks for the testing and explaining the context. This makes sense!

[Hallberg-NOAA]

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26703.

[marshallward]

I should have thought to provide this report:

   -> (1898,12) MOM_BAROTROPIC::BTLOOP_UPDATE_V (isz = 1595) (sz = 1690)         
      [[ Inlining would exceed -inline-max-total-size value (3099>2000) <3>]]    
   -> (1908,12) MOM_BAROTROPIC::BTLOOP_UPDATE_U (isz = 1437) (sz = 1530)         
      [[ Inlining would exceed -inline-max-total-size value (2941>2000) <3>]]                                                                                                          
   -> (1919,12) MOM_BAROTROPIC::BTLOOP_UPDATE_U (isz = 1437) (sz = 1530)         
      [[ Inlining would exceed -inline-max-total-size value (2941>2000) <3>]]    
   -> (1928,12) MOM_BAROTROPIC::BTLOOP_UPDATE_V (isz = 1595) (sz = 1690)         
      [[ Inlining would exceed -inline-max-total-size value (3099>2000) <3>]]

Compiler won't inline into a function larger than 2000 lines(? or something line-like...).

In other words, there is no problem with the new functions, but btstep is still a very large function!

This 2k line limit is tunable, but we should probably aim to refactor towards a number far away from this limit at some point in the future.