-
Notifications
You must be signed in to change notification settings - Fork 864
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
Conversation
@cparrott73 can you please give it a try? you should now be able to use |
@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:
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:
Let me know if I need to tweak something here. |
That sounds right, I also use auto tools provided by RHEL8 so the patch should be applied correctly. |
@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. |
I confirm it works on Rocky 9.4 as well both x86 and aarch64. |
@cparrott73 since we try to patch the generated |
Refactor SVE functions and incidentally make NVIDIA compilers a happy panda again. Signed-off-by: Gilles Gouaillardet <[email protected]>
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.
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
$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' | ||
;;"; |
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.
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.
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.
Can't easily because of the protections in $search_string
.
@ggouaillardet @jsquyres can we get this merged? |
60e745e
to
237f492
Compare
I tested again on x86 and aarch64 and it works with NVIDIA toolchains 24.7 and 24.9 (as long as |
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]>
237f492
to
ccd6415
Compare
@bosilca should we cherry-pick this to v5.0.x as well? |
Yes, probably so. Do you want it in v4.1.x, too? |
@jsquyres we should have it there as well |
nvfortran needs to be passed -fPIC when building shared libraries, so patch the generated configure script in order to properly handle nvfortran:
Refs. #8919