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

v4.1.x: Fix inconsistent configure-time check #11857

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

skosukhin
Copy link
Contributor

@skosukhin skosukhin commented Aug 15, 2023

This fixes the inconsistency between what is tested at the configure time and what is actually used in the source code. Note that the issue was solved in the main with #9445. As far as I understand, there are no plans to backport that PR to 4.1.x because it potentially changes the ABI compatibility. Therefore, this PR fixes the problem the other way around.

Starting version 23.3, NVHPC compiler supports type(*) and the configure script sets ompi_cv_fortran_ignore_tkr_data to 1:type(*), dimension(*):!GCC$ ATTRIBUTES NO_ARG_CHECK ::, which makes the mpi Fortran module unusable in some cases. For example, we cannot compile the following:

program main
  use mpi
  implicit none

  real(kind=selected_real_kind(12, 307)):: buf
  integer :: count, datatype, dest, tag, comm, ierror

  call mpi_send(buf, count, datatype, dest, tag, comm, ierror)
end program main

The compiler reports:

NVFORTRAN-S-0155-Could not resolve generic procedure mpi_send (./test2.f90: 8)
  0 inform,   0 warnings,   1 severes, 0 fatal for main

As was mentioned in #9445, the unnamed interfaces are more forgiving when it comes to TKR mismatches. Indeed, the code snippet run by the configure script uses the unnamed interface and is too weak for the following use case. This PR makes the configure-time check stricter by switching to the named interface, which is what is actually used in the code. With this change, the configure script sets ompi_cv_fortran_ignore_tkr_data to 1:real, dimension(*):!DIR$ IGNORE_TKR when using NVHPC 23.3+ (the same value as for the older versions of the compiler) and the code snipped above compiles successfully.

bot:notacherrypick

@github-actions github-actions bot added this to the v4.1.6 milestone Aug 15, 2023
@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

c3fbe69: Fix inconsistent configure-time check

  • check_signed_off: does not contain a valid Signed-off-by line
  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@skosukhin skosukhin force-pushed the fix_config_ignore_tkr branch from c3fbe69 to 044adbe Compare August 15, 2023 17:59
@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

044adbe: Fix inconsistent configure-time check

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@skosukhin skosukhin force-pushed the fix_config_ignore_tkr branch from 044adbe to 0f8e73b Compare August 15, 2023 18:02
@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

0f8e73b: Fix inconsistent configure-time check

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

1 similar comment
@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

0f8e73b: Fix inconsistent configure-time check

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@jsquyres
Copy link
Member

@skosukhin Can you add more information to the commit message about the "what" and "why" this commit is necessary? You put a lot of information in the PR body, which is great -- at least some of that should probably also be in the git commit message.

I understand your rationale for why this is not a cherry pick; you need to put the notacherrypick bot token in the PR description, not the git commit message. Then the CI bot will pass.

@jsquyres jsquyres self-requested a review August 21, 2023 15:14
@jsquyres jsquyres changed the title Fix inconsistent configure-time check v4.1.x: Fix inconsistent configure-time check Aug 21, 2023
@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

0f8e73b: Fix inconsistent configure-time check

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

This fixes the inconsistency between what is tested at the configure time (the
unnamed Fortran interface) and what is actually used in the source code (the
named Fortran interface).

As was mentioned in open-mpi#9445, the unnamed interfaces are more forgiving when it
comes to TKR mismatches. Therefore, the current configure-time check is not
strict enough and is prone to giving false results (e.g. for NVHPC 23.3+). The
fix is to switch to the named Fortran interface in the configure script.

Note that the inconsistency was resolved in the main branch with open-mpi#9445, which we
cannot cherry-pick because it potentially breaks the ABI compatibility.

Signed-off-by: Sergey Kosukhin <sergey.kosukhin@mpimet.mpg.de>
@skosukhin skosukhin force-pushed the fix_config_ignore_tkr branch from 0f8e73b to 5b4ee66 Compare August 21, 2023 15:33
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jsquyres jsquyres merged commit 3396fe2 into open-mpi:v4.1.x Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants