-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix: remove pointer attribute from tides_CS in unsplit mode #103
Bugfix: remove pointer attribute from tides_CS in unsplit mode #103
Conversation
Remove the pointer attribute from tides_CS in its parent CS in unsplit mode. This recovers tidal forcing functionality in the unsplit mode.
Also, it looks like the description comments above the child CSs ("A pointer to the ... ") and the variable names |
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #103 +/- ##
=========================================
Coverage 28.75% 28.75%
=========================================
Files 248 248
Lines 72970 72970
=========================================
Hits 20979 20979
Misses 51991 51991
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This has passed TC testing and pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15195. |
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 Not intending to turn this into a discussion, but I also encountered this warning recently from GCC 11:
meaning that I probably took things too far already and we need to reconsider some of the decisions. |
@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 |
@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 ( 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 🤣) |
@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. |
@marshallward Thanks for the explanation, this is really informative! |
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...)