Update configure's default compilers list #1024

victorapm · 2023-12-06T23:58:26Z

This PR updates the default compilers list used in the configure script such that:

The newer icx has precedence over icc
mpiicx/mpiicpx have precedence over mpiicc/mpiicpc
--enable-debug uses -O0 -g -Wall as CFLAGS

Closes #1021

liruipeng · 2023-12-11T16:19:44Z

Not sure about the changes. Please see my comments. Thanks!

src/config/hypre_macros_misc.m4

liruipeng · 2023-12-11T16:22:03Z

src/config/configure.in

@@ -1720,9 +1720,9 @@ then
   else
      if test "$hypre_using_openmp" = "yes"
      then
-         AC_CHECK_PROGS(CC, [mpxlc mpixlc_r mpixlc mpiicc mpiicx mpigcc mpicc mpipgcc mpipgicc])
+         AC_CHECK_PROGS(CC, [mpicc mpxlc mpixlc_r mpixlc mpiicc mpiicx mpigcc mpipgcc mpipgicc])


This changes the behavior of hypre's configure at LLNL machines, such as lassen, where xlc is picked by default.

mpicc is the same as mpixlc on Lassen. So this change seems fine to me:

vicmagri@lassen708 ~ $ mpixlc --version IBM XL C/C++ for Linux, V16.1.1 (5725-C73, 5765-J13) Version: 16.01.0001.0014 vicmagri@lassen708 ~ $ mpicc --version IBM XL C/C++ for Linux, V16.1.1 (5725-C73, 5765-J13) Version: 16.01.0001.0014

The main reason for this change is that mpicc is more general, so it makes sense to test for it first.

They may not always be the same, under different module loads. If one wants to test mpicc, they can always do CC=mpicc. I think my question is still: why change given that this works fine for long time? :)

I think I agree with @liruipeng here that we should not reorder these. This will change the behavior for many existing users.

Thanks for the comments! One issue with the current approach (specialized compilers first on the list) is that it doesn't guarantee that the most recent compiler loaded via module load will be the one picked by configure.

For example, on lassen, if we do module load clang and run configure, it will pick CC=mpixlc instead of CC=mpiclang. Moving mpicc to the top of the list is a solution to this problem (mpicc is effectively mpiclang in this scenario). Another solution, as we talked, is to explicitly set CC=mpiclang.

I don't see this as a big problem, because we have workarounds, but I just wanted to share it with you both...

liruipeng · 2023-12-11T16:25:34Z

src/config/configure.in

@@ -1738,16 +1738,16 @@ then
   then
      if test "$hypre_using_openmp" = "yes"
      then
-         AC_CHECK_PROGS(CXX, [xlC_r xlc_r xlC xlc icpc icc icpx icx g++ gcc pgCC pgcc pgc++ CC cc KCC kcc])
+         AC_CHECK_PROGS(CXX, [xlC_r xlc_r xlC xlc icx icpc icc icpx g++ gcc pgCC pgcc pgc++ CC cc KCC kcc])


Even though icc is gone, icx can be picked from the list. Not sure why we need to reorder.

Some systems have a residual icc installation lying around. If we test for icx first, the residual icc wouldn't be a problem.

Again, they can always do CC=WHATEVER, and after icc is complete gone, we just delete it from the list.

I agree users can always specify the compiler they want via CC, however the main aim here was to make the process of choosing compilers automatically (by the configure script) more robust. If such an improvement isn't necessary, I can close the PR :) @rfalgout thoughts?

Hi @victorapm I don't really have that strong opinion on this PR :) My main concern if it is worth our time to change and test for some thing that is working OK. I am a "minimalist" :). Thanks!

If we are reordering 'icc' (paired with 'icpc') and 'icx' (paired with 'icpx'), why are we not also reordering 'mpiicc' and 'mpiicx'? I am not opposed to switching the order of these if 'icc' is going away, but shouldn't we do this consistently? We tried to keep the mpi versions consistent with the raw compilers before.

Good catch. mpiicc and mpiicx should be reordered as well. I can do that if we decide this PR is useful

Okay, since the two compilers are right next to each other in the list, if we switch them, it should be "in place" and look as follows:

icpx icx icpc icc mpiicx mpiicc

I don't know what the 'p' compilers are, but they were needed at some point so let's keep them there and in the order they are already given with respect to the non-'p' versions.

I am okay with making this change. @liruipeng , what do you think?

I don't think we should move mpicc to a new position, however. In general, the compiler orders here were originally intended to grab the most useful compiler on the LLNL systems first, so the general compilers tended to be near the end not the front. But that was a long time ago and now it's more disruptive to change the orders of these things too much.

icpx icx icpc icc mpiicx mpiicc

looks good to me.

Done in 91583f1

rfalgout · 2023-12-11T17:13:27Z

Hi @victorapm and @liruipeng . Both of you make good points, but let me make a general comment before replying individually to some of the changes. The magic of configure or cmake or spack automatically finding compilers and such is nice to have, but it will not always work or work perfectly for every user. So, we need to try to make it work for as many users as possible, while minimizing the impact on existing users. This is a subjective call and I think most of your conversation has been about balancing these two conflicting goals.

rfalgout · 2023-12-11T17:47:48Z

src/config/hypre_macros_misc.m4

@@ -220,31 +220,31 @@ if test "x${hypre_user_chose_cflags}" = "xno"
 then
   case `basename ${CC}` in
      gcc|mpigcc|mpicc)
-        CFLAGS="-g -Wall"
+        CFLAGS="-O0 -g -Wall"


I don't think we should add the '-O0' to '--enable-debug'. I think this should just turn on debugging, not modify the optimization setting. Why do you think this is a good thing to always do here? Sometimes it is good to debug with different optimization settings (the bug may be caused by the optimization setting).

Good catch. I didn't see this change since I thought they are all the "format changes". We also have -O0 in CUFLAG with --enable-debug, which may not be a good thing either, see

hypre/src/config/configure.in

Line 2543 in 70e9dec

CUFLAGS="-g -O0 ${CUFLAGS}"

. The reason I added was optimization sometimes can remove information used by debuggers. I usually use debuggers like gdb with --enable-debug.

Because the compiler is free to choose the optimization level if not specified by the user. In many cases, the optimization level chosen by default is -O2. However, when debugging with gdb, -O2 generally hides the value of local variables, which doesn't help debugging.

The point of a bug being related to a particular optimization flag is good. If that is the case, the developer trying to understand the bug could do --with-extra-CFLAGS="-O2", for example, and O2 would have the priority, thus bringing back the bug again

Let's get a consensus on this question via teams. For me, I use '--enable-debug' all the time and almost never have to additionally change the optimization flag (when I have done that in the past, it was typically because the optimization setting was causing the bug in the first place). I usually use totalview, not gdb. So, this will change the behavior of '--enable-debug' for me and I'm not sure if it will be for better or worse until I start using it. My guess is I won't notice anything at all.

Whatever we decide, we should be consistent.

the compiler is free to choose the optimization level [...] In many cases [...] default is -O2.

I did some research to clarify this and found the following defaults:

Compiler Opt. level

icx/icc -O2

xlc -O2

gcc -O0

clang -O0

msvc -O0

I think it would be good to have an expected behavior with --enable-debug, either by ensuring -O0 or -O2.

Also, CMake sets -O0 with its -DCMAKE_BUILD_TYPE=Debug option. So, I think it makes sense to get the same with configure's --enable-debug for consistency.

This seems reasonable to me. @liruipeng , what do you think?

victorapm · 2023-12-17T17:06:20Z

@rfalgout Does it make sense to remove insure (for the purpose of some cleaning up)? See:

hypre/src/config/configure.in

Lines 2106 to 2124 in 3352499

    
           dnl ********************************************************************* 
        
           dnl * Set INSURE options 
        
           dnl ********************************************************************* 
        
           if test "$hypre_using_insure" = "yes" 
        
           then 
        
           dnl   LINK_F77="insure" 
        
              LINK_FC="insure" 
        
              LINK_CC="insure" 
        
              LINK_CXX="insure" 
        
              LDFLAGS=`mpicc -link-info | awk '{$1=""; print}'` 
        
              LDFLAGS="$LDFLAGS ${hypre_insure_flags}" 
        
           dnl   F77="insure" 
        
              FC="insure" 
        
              CC="insure" 
        
              CXX="insure" 
        
              FFLAGS="`mpicc -link-info | awk '{$1=""; print}'` $FFLAGS" 
        
              CFLAGS="`mpicc -link-info | awk '{$1=""; print}'` $CFLAGS" 
        
              CXXFLAGS="`mpicc -link-info | awk '{$1=""; print}'` $CXXFLAGS" 
        
           fi

rfalgout

This looks good to me. How does it affect the regression tests?

rfalgout · 2023-12-19T21:11:28Z

@rfalgout Does it make sense to remove insure (for the purpose of some cleaning up)?

Yes, we could probably do that without upsetting anyone. Thanks!

* The newer icx has precedence over icc * mpiicx/mpiicpx have precedence over mpiicc/mpiicpc * --enable-debug uses "-O0 -g -Wall" as CFLAGS * Remove insure support

victorapm added 3 commits December 6, 2023 18:33

Update default compilers list

358f1fd

Remove cc from the list

e31b17e

Update default compilation flags

0fd6416

victorapm linked an issue Dec 6, 2023 that may be closed by this pull request

configure can't find Intel icx compiler #1021

Closed

victorapm requested review from liruipeng and rfalgout December 9, 2023 04:19

victorapm changed the title ~~[WIP] Update configure's default compilers list~~ Update configure's default compilers list Dec 9, 2023

liruipeng reviewed Dec 11, 2023

View reviewed changes

src/config/hypre_macros_misc.m4 Show resolved Hide resolved

liruipeng reviewed Dec 11, 2023

View reviewed changes

rfalgout reviewed Dec 11, 2023

View reviewed changes

Revert mpicc position + Prioritize mpiicpx over mpiicpc

91583f1

rfalgout approved these changes Dec 19, 2023

View reviewed changes

liruipeng approved these changes Dec 20, 2023

View reviewed changes

victorapm added 2 commits December 19, 2023 19:37

Remove insure support

00f9056

Do not use -Wall in debug mode by default for mpixlC

0d5ea16

victorapm merged commit 17fa100 into master Dec 21, 2023

victorapm deleted the fix-icx branch December 21, 2023 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update configure's default compilers list #1024

Update configure's default compilers list #1024

victorapm commented Dec 6, 2023 •

edited

Loading

liruipeng commented Dec 11, 2023

liruipeng Dec 11, 2023

victorapm Dec 11, 2023

liruipeng Dec 11, 2023

rfalgout Dec 11, 2023

victorapm Dec 11, 2023

liruipeng Dec 11, 2023

victorapm Dec 11, 2023

liruipeng Dec 11, 2023

victorapm Dec 11, 2023

liruipeng Dec 11, 2023

rfalgout Dec 11, 2023

victorapm Dec 11, 2023

rfalgout Dec 11, 2023

liruipeng Dec 11, 2023

victorapm Dec 17, 2023

rfalgout commented Dec 11, 2023 •

edited by victorapm

Loading

rfalgout Dec 11, 2023

liruipeng Dec 11, 2023

victorapm Dec 11, 2023 •

edited

Loading

rfalgout Dec 11, 2023

rfalgout Dec 11, 2023

victorapm Dec 17, 2023 •

edited

Loading

rfalgout Dec 19, 2023

victorapm commented Dec 17, 2023

rfalgout left a comment

rfalgout commented Dec 19, 2023

Update configure's default compilers list #1024

Update configure's default compilers list #1024

Conversation

victorapm commented Dec 6, 2023 • edited Loading

liruipeng commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfalgout commented Dec 11, 2023 • edited by victorapm Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorapm Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorapm Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorapm commented Dec 17, 2023

rfalgout left a comment

Choose a reason for hiding this comment

rfalgout commented Dec 19, 2023

victorapm commented Dec 6, 2023 •

edited

Loading

rfalgout commented Dec 11, 2023 •

edited by victorapm

Loading

victorapm Dec 11, 2023 •

edited

Loading

victorapm Dec 17, 2023 •

edited

Loading