Skip to content
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

Some bounds assertions aren't done when threading is on #811

Closed
billsacks opened this issue Oct 3, 2019 · 3 comments
Closed

Some bounds assertions aren't done when threading is on #811

billsacks opened this issue Oct 3, 2019 · 3 comments
Assignees
Labels
priority: low Background task that doesn't need to be done right away. tag: simple bfb easy to fix, bit-for-bit type: code cleanup improving internal code structure

Comments

@billsacks
Copy link
Member

billsacks commented Oct 3, 2019

In these two blocks of code:

#ifndef _OPENMP
if ( present(c13) .and. use_c13 )then
SHR_ASSERT_ALL((lbound(c13) == (/bounds%begp/)), 'lbnd(c13)'//errMsg(sourcefile, lineno))
SHR_ASSERT_ALL((ubound(c13) == (/bounds%endp/)), 'ubnd(c13)'//errMsg(sourcefile, lineno))
end if
if ( present(c14) .and. use_c14 )then
SHR_ASSERT_ALL((lbound(c14) == (/bounds%begp/)), 'lbnd(c14)'//errMsg(sourcefile, lineno))
SHR_ASSERT_ALL((ubound(c14) == (/bounds%endp/)), 'ubnd(c14)'//errMsg(sourcefile, lineno))
end if
#endif

#ifndef _OPENMP
if ( present(c13) .and. use_c13 )then
SHR_ASSERT_ALL((lbound(c13) == (/bounds%begp/)), errMsg(sourcefile, __LINE__))
SHR_ASSERT_ALL((ubound(c13) == (/bounds%endp/)), errMsg(sourcefile, __LINE__))
end if
if ( present(c14) .and. use_c14 )then
SHR_ASSERT_ALL((lbound(c14) == (/bounds%begp/)), errMsg(sourcefile, __LINE__))
SHR_ASSERT_ALL((ubound(c14) == (/bounds%endp/)), errMsg(sourcefile, __LINE__))
end if
#endif

the bounds of subroutine arguments are not being checked if threading is on. It looks like this came in in clm4_5_9_r184. Presumably that was needed at some point, but I hope we can remove this now: if this is needed, I think that would suggest a threading bug.

@ekluzek I'm assigning this to you since you made clm4_5_9_r184, but it probably isn't high priority. The fix should be simple (removing the #ifdefs) as long as it doesn't cause problems.

@billsacks billsacks added priority: low Background task that doesn't need to be done right away. type: code cleanup improving internal code structure tag: simple bfb easy to fix, bit-for-bit labels Oct 3, 2019
@ekluzek
Copy link
Contributor

ekluzek commented Oct 17, 2019

Yes, it looks like to me that I should be able to remove the OMP idef's and it should work correctly. So we'll have to try it and make sure it does.

@negin513
Copy link
Contributor

Removing the #ifndef _OPENMPgive us error in lbound and ubound assertions for some of the threading tests. After a discussion with @billsacks today, we decided to remove the pointer attribute of c13 and c14. In all Truncate**** functions (e.g. TruncateCandNStates, TruncateNStates, etc.) , all arrays should have lbound and ubound in the slices – this includes the existing ones that have just lower bound in addition to c13 and c14 which currently don't have any bounds. We also have to remove lbound assertions for c13 and c14. These changes seem to resolve the errors of threading tests.

negin513 added a commit to negin513/ctsm that referenced this issue Jan 29, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 5, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 5, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
negin513 added a commit to negin513/ctsm that referenced this issue Feb 6, 2020
billsacks added a commit that referenced this issue Feb 20, 2020
Bounds assertion for C isotopes when threading is on

Previously, the bounds of c13 and c14 in subroutines TruncateCStates and
TruncateCandNStates were not checked when threading was on. This PR allows us
to handle c13 and c14 arrays like everything else in CTSM including (bounds
checking).

This PR resolves issue #811
@billsacks billsacks added this to Master Tags/Issues Done in Upcoming tags Feb 20, 2020
@billsacks
Copy link
Member Author

Fixed by @negin513 in ctsm1.0.dev084

ekluzek added a commit to ekluzek/CTSM that referenced this issue Feb 25, 2020
Bounds assertion for C isotopes when threading is on

Previously, the bounds of c13 and c14 in subroutines TruncateCStates and
TruncateCandNStates were not checked when threading was on. This PR allows us
to handle c13 and c14 arrays like everything else in CTSM including (bounds
checking).

This PR resolves issue ESCOMP#811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low Background task that doesn't need to be done right away. tag: simple bfb easy to fix, bit-for-bit type: code cleanup improving internal code structure
Projects
Upcoming tags
Done (non release/external)
Development

Successfully merging a pull request may close this issue.

3 participants