Bugfix: remove pointer attribute from tides_CS in unsplit mode

[herrwang0]

This just seems to be an omission from the de-pointer makeover. The pointer attribute in tides_CS was left unchanged in both RK2 and RK3 unsplit mode.

This simple PR patches that and allows tides to be used in unsplit mode again.

(sorry for making such a short PR, but this does affect functionality...)

[herrwang0]

Also, it looks like the description comments above the child CSs ("A pointer to the ... ") and the variable names CSp may need to be modified in the future.

[codecov]

Codecov Report

Merging #103 (bfd10df) into dev/gfdl (28ff86e) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           dev/gfdl     #103   +/-   ##
=========================================
  Coverage     28.75%   28.75%           
=========================================
  Files           248      248           
  Lines         72970    72970           
=========================================
  Hits          20979    20979           
  Misses        51991    51991           

Impacted Files	Coverage Δ
src/core/MOM_dynamics_unsplit.F90	64.42% <ø> (ø)
src/core/MOM_dynamics_unsplit_RK2.F90	63.13% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28ff86e...bfd10df. Read the comment docs.

[Hallberg-NOAA]

I agree that these changes are consistent with the changes elsewhere in the code about 6 months ago, and that they should be accepted as soon as the standard tests have passed.

[Hallberg-NOAA]

This has passed TC testing and pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15195.

[marshallward]

Thank you @herrwang0, I agree these appear to have been overlooked, probably because we almost never use them.

The broader goal of pointer phaseout was put on pause as the reliance on associated() checks became more difficult to navigate.

Not intending to turn this into a discussion, but I also encountered this warning recently from GCC 11:

Warning: Array 'mom_csp' at (1) is larger than limit set by '-fmax-stack-var-size=', moved from stack to static storage. This makes the procedure unsafe when called recursively, or concurrently from multiple threads. Consider using '-frecursive', or increase the '-fmax-stack-var-size=' limit, or change the code to use an ALLOCATABLE array. [-Wsurprising]

meaning that I probably took things too far already and we need to reconsider some of the decisions.

[herrwang0]

@marshallward Thanks for the response.

That's interesting. My understanding is that the pointer CS were replaced to gain contiguity. It seems that in this case the compiler for some reason thinks the stack is increased? Or that maybe some child CS of MOM_CSp is accidentally not allocatable?

[marshallward]

@herrwang0 By replacing these pointers with local instances, we did implicitly move them from heap to stack. (Strictly, this is at the discretion of the compiler, but is a safe assumption.)

This was deliberate, but also a bit of a gamble. Pointers are terrible for contiguity since their position can change at runtime, and the compiler must assume this possibility. Allocatables are much better, but can still be defined externally so may not be aligned or even contiguous. (This is a place where some compilers will step in and enforce contiguity, btw.). But stack is the safest choice, because the compiler has complete control over locals, and gives us the best chance for an ideal memory layout.

But it also exhausts our stack, which is usually a very limited resource. This is what the compiler is warning us about. These derived types are very large, containing lots of arrays, so the warning is very real.

In practice, we use the stack so aggressively (limit stacksize unlimited) that I hoped we could get away with it. And I guess we have, since no one has complained yet. But the compiler is telling us that we should probably reconsider.

If we do find that stack usage is a problem, then I think we could redefine these local derived types as allocatables.

(Sorry for that rather long explanation, I suppose this is starting to get a bit off topic 🤣)

[marshallward]

@herrwang0 Another point I forgot to mention. Since most of our CS's contain allocatable arrays, I had hoped that the actual array data would remain in heap, while the pointers inside of the CS's could be safely moved to stack.

This is probably still true, but these are still very large derived types with a lot of fields inside of them, perhaps even containing a few arrays here and there. This could be causing the total stack usage to increase more than we expect, and trigger this warning.

[herrwang0]

@marshallward Thanks for the explanation, this is really informative!