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

configury: patch configure to support nvfortran #12722

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

ggouaillardet
Copy link
Contributor

nvfortran needs to be passed -fPIC when building shared libraries, so patch the generated configure script in order to properly handle nvfortran:

  • add nvfortran to the list of known fortran compilers
  • pass -fPIC to the compiler

Refs. #8919

@ggouaillardet
Copy link
Contributor Author

@cparrott73 can you please give it a try?

you should now be able to use nvfortran without having to manually pass FCFLAGS=-fPIC to configure.
Once you apply the patch, you will have to run ./autogen.pl (or ./autogen.pl --force if you apply the patch on an official tarball.

@cparrott73
Copy link

@ggouaillardet - I tried the patch with nvfortran here, but ./configure is still not passing -fPIC by default for me here.

Here is what I did:

  • Clone ompi from git via git clone --recursive https://github.com/open-mpi/ompi.git
  • Downloaded the above patch to a raw .diff file
  • cd ompi then patch -p1 < fpic_fix.diff
  • ./autogen.pl --force
  • mkdir build && cd build
  • ../configure CC=nvc FC=nvfortran CXX=nvc++

Did I not do something right here?

The system in question is a Supermicro H11DSU-iN AMD EPYC 7452 system running Rocky Linux 8.10. The following relevant packages are installed:

cparrott@rome5 /scratch/cparrott/ompi_fpic_test/ompi/build $ rpm -qa | grep autoconf
autoconf-2.69-29.el8.noarch
cparrott@rome5 /scratch/cparrott/ompi_fpic_test/ompi/build $ rpm -qa | grep automake
automake-1.16.1-8.el8.noarch

Let me know if I need to tweak something here.

@ggouaillardet
Copy link
Contributor Author

That sounds right, I also use auto tools provided by RHEL8 so the patch should be applied correctly.
Can you please compress and upload configure, config.log and libtool so I can have a look?

@cparrott73
Copy link

@ggouaillardet - I tried this again while ago. I think my previous patch file from above may have gotten corrupted somehow. I pulled a new copy of the source tree from git, and then a new copy of the patch from above, and applied it, then ran ./configure and make. Everything worked as expected this time.

Interestingly, I also tried this on our older CentOS 7 build system (due to be retired soon). The change did not work as expected there. I'm wondering if this could be due to bugs/issues with autoconf and automake on that particular OS?

I also made a version of this patch for openmpi-4.1.5 by stripping out the first hunk, where the patch fails because of substantial changes in the copyright text since that version. This also worked okay for me on our Rocky 8 system.

All in all, I think we're good to go, and this experience has been educational to me - particularly with some of the shortcomings of CentOS 7.

@bosilca
Copy link
Member

bosilca commented Aug 1, 2024

I confirm it works on Rocky 9.4 as well both x86 and aarch64.

@ggouaillardet
Copy link
Contributor Author

@cparrott73 since we try to patch the generated configure and silently fail if the patch does not apply, the libtool.m4 used to generate configure must have the same bits than the one used to generate the patch.
I generated it from a RHEL8 like box. If you want to use CentOS7, I suggest you manually apply the commit, autogen.pl and then make dist on a RHEL8 box, and then build from the tarball on CentOS7.

ggouaillardet referenced this pull request Aug 2, 2024
Refactor SVE functions and incidentally make NVIDIA compilers
a happy panda again.

Signed-off-by: Gilles Gouaillardet <[email protected]>
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.

Other than my (optional) small suggestion, @cparrott73 confirmed that it works and @bosilca also gave a 👍. Let's move this out of draft and get it merged.

autogen.pl Outdated
Comment on lines 1097 to 1108
$replace_string = "nvfortran*)
# NVIDIA Fortran compiler
lt_prog_compiler_wl${tag}='-Wl,'
lt_prog_compiler_pic${tag}='-fPIC'
lt_prog_compiler_static${tag}='-Bstatic'
;;
tcc*)
# Fabrice Bellard et al's Tiny C Compiler
lt_prog_compiler_wl='-Wl,'
lt_prog_compiler_pic='-fPIC'
lt_prog_compiler_static='-static'
;;";
Copy link
Member

Choose a reason for hiding this comment

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

Small suggestion for the $replace_string:

$replace_string = "$search_string
    nvfortran*)
	# NVIDIA Fortran compiler
	lt_prog_compiler_wl${tag}='-Wl,'
	lt_prog_compiler_pic${tag}='-fPIC'
	lt_prog_compiler_static${tag}='-Bstatic'
	;;";

That way you don't have to replicate the tcc*) block twice.

Copy link
Member

Choose a reason for hiding this comment

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

Can't easily because of the protections in $search_string.

@janjust
Copy link
Contributor

janjust commented Aug 15, 2024

@ggouaillardet @jsquyres can we get this merged?

@bosilca bosilca self-assigned this Oct 22, 2024
@bosilca bosilca marked this pull request as ready for review October 22, 2024 15:35
@bosilca
Copy link
Member

bosilca commented Oct 22, 2024

I tested again on x86 and aarch64 and it works with NVIDIA toolchains 24.7 and 24.9 (as long as --enable-alt-short-float=no is passed to configure on aarch64).

nvfortran needs to be passed -fPIC when building shared libraries,
so patch the generated configure script in order to properly
handle nvfortran:
- add nvfortran to the list of known fortran compilers
- pass -fPIC to the compiler

Refs. open-mpi#8919

Signed-off-by: Gilles Gouaillardet <[email protected]>
Signed-off-by: George Bosilca <[email protected]>
@bosilca bosilca merged commit 25942c7 into open-mpi:main Oct 23, 2024
15 checks passed
@bosilca bosilca deleted the topic/nvfortran branch October 23, 2024 03:00
@janjust
Copy link
Contributor

janjust commented Oct 23, 2024

@bosilca should we cherry-pick this to v5.0.x as well?

@jsquyres
Copy link
Member

@bosilca should we cherry-pick this to v5.0.x as well?

Yes, probably so.

Do you want it in v4.1.x, too?

@janjust
Copy link
Contributor

janjust commented Oct 23, 2024

@jsquyres we should have it there as well

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.

5 participants