fail to link op/avx component with nvhpc compilers

[ggouaillardet]

As reported by Ray Muno on the user mailing list (https://www.mail-archive.com/users@lists.open-mpi.org/msg34594.html), op/avx component cannot be linked when the nvhpc compilers are used

 CCLD     liblocal_ops_avx512.la
   CCLD     mca_op_avx.la
./.libs/liblocal_ops_avx512.a(liblocal_ops_avx512_la-op_avx_functions.o):(.data+0x0): multiple 
definition of `ompi_op_avx_functions_avx2'
./.libs/liblocal_ops_avx2.a(liblocal_ops_avx2_la-op_avx_functions.o):(.data+0x0): first defined here
./.libs/liblocal_ops_avx512.a(liblocal_ops_avx512_la-op_avx_functions.o): In function 
`ompi_op_avx_2buff_min_uint16_t_avx2':
/project/muno/OpenMPI/BUILD/SRC/openmpi-4.1.1/ompi/mca/op/avx/op_avx_functions.c:651: multiple 
definition of `ompi_op_avx_3buff_functions_avx2'
./.libs/liblocal_ops_avx2.a(liblocal_ops_avx2_la-op_avx_functions.o):/project/muno/OpenMPI/BUILD/SRC/openmpi-4.1.1/ompi/mca/op/avx/op_avx_functions.c:651: 
first defined here
make[2]: *** [mca_op_avx.la] Error 2

The root cause is Open MPI assumes the following macros are defined if the compiler supports the AVX512 features we expect

__AVX512BW__
 __AVX512F__
__AVX512VL__

and at least one of these macros is not defined by nvhpc compilers

From ompi/mca/op/avx/op_avx_functions.c

#if defined(GENERATE_AVX512_CODE)
#  if defined(__AVX512BW__) && defined(__AVX512F__) && defined(__AVX512VL__)
#    define PREPEND _avx512
#  else
#    undef GENERATE_AVX512_CODE
#  endif  /* defined(__AVX512BW__) && defined(__AVX512F__) && defined(__AVX512VL__) */
#endif  /* defined(GENERATE_AVX512_CODE) */

I am wondering whether we really need to re-test these macros. if GENERATE_AVX512_CODE is defined, should we not be ready to go (and hence no more need to test the avx512 related macros)?

[jsquyres]

@ggouaillardet Is this related to #8919?

[bosilca]

There is a lengthy comment explaining why we need to test these macros in the .c file. Most compilers converge toward a well-defined, almost portable, support for immintrin.h, apparently with the exception of nvc. Until they fix the compiler, I propose to completely drop the generation of AVX code. Can you please try to following patch.

diff --git a/ompi/mca/op/avx/configure.m4 b/ompi/mca/op/avx/configure.m4
index 44e834301b..1e45624abb 100644
--- a/ompi/mca/op/avx/configure.m4
+++ b/ompi/mca/op/avx/configure.m4
@@ -123,6 +123,27 @@ AC_DEFUN([MCA_ompi_op_avx_CONFIG],[
                               MCA_BUILD_OP_AVX512_FLAGS=""
                               AC_MSG_RESULT([no])])
                          CFLAGS="$op_avx_cflags_save"
+                        ])
+                  #
+                  # Detect and drop AVX support for compilers that do not indicate
+                  # explicit AVX capabilities via defines (aka nvc at least before 21.9)
+                  #
+                  AS_IF([test $op_avx512_support -eq 1],
+                        [AC_MSG_CHECKING([if AVX512 defines are available])
+                         op_avx_cflags_save="$CFLAGS"
+                         CFLAGS="$CFLAGS_WITHOUT_OPTFLAGS -O0 $MCA_BUILD_OP_AVX512_FLAGS"
+                         AC_LINK_IFELSE(
+                             [AC_LANG_PROGRAM([[#include <immintrin.h>]],
+                                      [[
+#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
+#error "This compiler claims support for AVX512 but lacks the necessary #define
+#endif
+                                      ]])],
+                             [AC_MSG_RESULT([yes])],
+                             [op_avx512_support=0
+                              MCA_BUILD_OP_AVX512_FLAGS=""
+                              AC_MSG_RESULT([no])])
+                         CFLAGS="$op_avx_cflags_save"
                         ])])
            #
            # Check support for AVX2

[cparrott73]

The nvhpc compilers team is aware of this issue, but there are two separate things going on here.

nvhpc compilers by default generate code targeted for the host CPU, which is different behavior than gcc, clang, and possibly other compilers - which all target a base x86_64 instruction set by default, and only enable enhanced code generation via additional flags (e.g. -march, -mavx, etc.). This means that the Open MPI ./configure test to check "does the compiler support AVX-512 intrinsics without passing any flags" will get different results depending on whether you are running ./configure on a Sandy Bridge (AVX), Haswell/Broadwell (AVX2), or Xeon Skylake (AVX-512). I would suggest that this is not a particularly reliable test to use with nvhpc compilers, for this reason.

My suggestion would be to disable this test when nvhpc (nvc/nvfortran) compilers are used, and follow the same path that gcc takes once this test returns a negative answer for gcc: go on to test the -mavx, -mavx2, -mavx512* flags individually. nvhpc compilers support these same flags as of 21.9, and you can test on those to determine the correct flags to pass to the compiler to enable the appropriate AVX intrinsics for compiling each version of the op-avx module. This will ensure that these files get built with the correct symbols when using nvhpc compilers.

Related to this, there is also a bug in 21.9 where the compiler detects that the host supports AVX-512 (e.g. Xeon Skylake), but not all of the AVX-512 macros were being passed by default. This led directly to the linking error reported initially with this issue.

Hopefully this helps.

[bosilca]

I'm not sure I agree with your analysis about the check for additional compiler flags. What this part is doing is trying to figure out how to convince the compiler to generate code for particular flavors of the X86 ISA. In fact, we need to compile the same file 3 times, for AVX, AVX2 and AVX512 support, and then we merge everything together and decide at runtime which version to use based on cpuid. So the code you are pointing to only detect which one of the 3 cases is natively generated by the selected compiler and flags (because this generation includes the CFLAGS set by the user).

In any case the dealbreaker is that at compile time we will miss the AVX512 #define, and as a result we will compile the same file twice but with the same renaming scheme. Thus the safe approach is to disable AVX512 not only on what ISA the compiler promises to generate but also depending on the existence of the #define that drive our renaming.

[cparrott73]

Fair point, I may have misinterpreted what this particular test is trying to accomplish. I do normally test build libraries such as Open MPI with a "-tp px" flag for maximum portability, so we don't run into issues with illegal instructions on systems that don't match our build host (a Skylake Xeon system). Now that the bug has been fixed in the development tree of our compilers, I will double check that compiling Open MPI with this flag still works when performing the op/avx tests.

[cparrott73]

FYI I decided to test "-tp px" internally with our current development compiler build, but I have run into an unrelated issue. I have filed a bug on it. I will let you know when I am able to make progress again.

[bartoldeman]

I think this is what makes the difference: NVHPC's immintrin.h has this:

#if !(defined(_MSC_VER) || defined(__SCE__)) || __has_feature(modules) ||      \
    defined(__AVX512F__)
#include <avx512fintrin.h>
#endif

but GCC simply includes <avx512fintrin.h> which then does

#ifndef __AVX512F__
#pragma GCC push_options
#pragma GCC target("avx512f")
#define __DISABLE_AVX512F__
#endif /* __AVX512F__ */

NVHPC has no logic like this so it just compiles any avx512 intrinsics without issue (even if you use -tp px), i.e this test blurb:

#include <immintrin.h>

int main(void) {
    __m512 vA, vB;
    _mm512_add_ps(vA, vB);
    }

with -D__SCE__ it works ok (-D_MSC_VER not, I guess that does other things, pretending MSVC, I have no idea what SCE means).

$ nvc -tp skylake -D__SCE__ test.c
"test.c", line 8: warning: variable "vA" is used before its value is set
      _mm512_add_ps(vA, vB);
                    ^

"test.c", line 8: warning: variable "vB" is used before its value is set
      _mm512_add_ps(vA, vB);
                        ^
$ nvc -tp haswell -D__SCE__ test.c
"test.c", line 7: error: identifier "__m512" is undefined
      __m512 vA, vB;
      ^

"test.c", line 8: warning: function "_mm512_add_ps" declared implicitly
      _mm512_add_ps(vA, vB);
      ^

1 error detected in the compilation of "test.c".

it's an undocumented workaround so beware of demons...

[bosilca]

The real issue is not that the compiler always generates AVX512 code, but that it fails to define one of the AVX512BW or AVX512F or AVX512VL defines to let us know it will generate the code. And that's exactly what my patch above addresses. Can you try and let me know if it works ?

[bartoldeman]

The real issue is not that the compiler always generates AVX512 code, but that it fails to define one of the AVX512BW or AVX512F or AVX512VL defines to let us know it will generate the code. And that's exactly what my patch above addresses. Can you try and let me know if it works ?

Your patch does what it's intended to do. But what puzzled me is that it's not necessary for NVHPC 21.7, even though both 21.7 & 21.9 have the strange AVX512 flag behaviour.

Now diffing immintrin.h for 21.7 and 21.9 I see this:

-#if defined(__AVX512F__)
+#if !(defined(_MSC_VER) || defined(__SCE__)) || __has_feature(modules) ||      \
+    defined(__AVX512F__)
 #include <avx512fintrin.h>
 #endif

this explains why it compiles ok with 21.7, this checks are as follows:

checking for AVX512 support... yes
checking for AVX512 support (no additional flags)... no
checking for AVX512 support (with -mavx512f -mavx512bw -mavx512vl -mavx512dq)... yes
checking if _mm512_loadu_si512 generates code that can be compiled... yes
checking if _mm512_mullo_epi64 generates code that can be compiled... yes

but with 21.9 the first "no" turns into a "yes" because of the above header change.

21.7 and 21.9 BOTH do the following:

1. nvc -tp skylake: defines AVX512BW and AVX512F and AVX512VL
2. nvc -mavx512f -mavx512bw -mavx512vl -mavx512dq: defines all the above as well
3. nvc without -tp on a skylake CPU: does not define AVX512BW, but does define AVX512F. And that's the weird situation you're encountering here (and I believe this is a bug in NVHPC)

Now I also have a proposal to adjust your patch: instead of giving up, set -mavx512f -mavx512bw -mavx512vl -mavx512dq. This can probably be done by changing this check, earlier in that file, to be more general:

#if defined(__ICC) && !defined(__AVX512F__)
#error "icc needs the -m flags to provide the AVX* detection macros
#endif

could be generalized to:

#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
#error "compiler needs the -m flags to provide the AVX* detection macros"
#endif

[bosilca]

Interesting suggestion, it should indeed address all issues we were covering in this discussion. How about the following patch ?

diff --git a/ompi/mca/op/avx/configure.m4 b/ompi/mca/op/avx/configure.m4
index 44e834301b..223dd8207e 100644
--- a/ompi/mca/op/avx/configure.m4
+++ b/ompi/mca/op/avx/configure.m4
@@ -50,8 +50,8 @@ AC_DEFUN([MCA_ompi_op_avx_CONFIG],[
                   AC_LINK_IFELSE(
                       [AC_LANG_PROGRAM([[#include <immintrin.h>]],
                                        [[
-#if defined(__ICC) && !defined(__AVX512F__)
-#error "icc needs the -m flags to provide the AVX* detection macros"
+#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
+#error "compiler needs the -m flags to provide the AVX* detection macros"
 #endif
     __m512 vA, vB;
     _mm512_add_ps(vA, vB)
@@ -67,8 +67,8 @@ AC_DEFUN([MCA_ompi_op_avx_CONFIG],[
                          AC_LINK_IFELSE(
                              [AC_LANG_PROGRAM([[#include <immintrin.h>]],
                                               [[
-#if defined(__ICC) && !defined(__AVX512F__)
-#error "icc needs the -m flags to provide the AVX* detection macros"
+#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
+#error "compiler needs the -m flags to provide the AVX* detection macros"
 #endif
     __m512 vA, vB;
     _mm512_add_ps(vA, vB)
@@ -90,8 +90,8 @@ AC_DEFUN([MCA_ompi_op_avx_CONFIG],[
                          AC_LINK_IFELSE(
                              [AC_LANG_PROGRAM([[#include <immintrin.h>]],
                                       [[
-#if defined(__ICC) && !defined(__AVX512F__)
-#error "icc needs the -m flags to provide the AVX* detection macros"
+#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
+#error "compiler needs the -m flags to provide the AVX* detection macros"
 #endif
     int A[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
     __m512i vA = _mm512_loadu_si512((__m512i*)&(A[1]))
@@ -112,8 +112,8 @@ AC_DEFUN([MCA_ompi_op_avx_CONFIG],[
                          AC_LINK_IFELSE(
                              [AC_LANG_PROGRAM([[#include <immintrin.h>]],
                                       [[
-#if defined(__ICC) && !defined(__AVX512F__)
-#error "icc needs the -m flags to provide the AVX* detection macros"
+#if !defined(__AVX512BW__) || !defined(__AVX512F__) || !defined(__AVX512VL__)
+#error "compiler needs the -m flags to provide the AVX* detection macros"
 #endif
     __m512i vA, vB;
     _mm512_mullo_epi64(vA, vB)

[bartoldeman]

Finally got back to this. Bad news, with the above patch it still doesn't compile; it fails here:

  CCLD     ompi_info
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pmaxu.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pmins.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pmaxs.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pmaxu.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pminu.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pminu.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: ../../../ompi/.libs/libmpi.so: undefined reference to `llvm.x86.avx512.pmins.q'

I reduced it to an example with these 8 AVX512 functions:

#include <immintrin.h>

int main(int argc, char **argv)
{
    __m512i vecA = _mm512_loadu_si512((__m512 *)argv[0]);
    __m512i vecB = _mm512_loadu_si512((__m512 *)argv[1]);
    vecB = _mm512_max_epi64(vecA, vecB);
    vecB = _mm512_min_epi64(vecA, vecB);
    vecB = _mm512_max_epu64(vecA, vecB);
    vecB = _mm512_min_epu64(vecA, vecB);
    vecB = _mm512_max_epi32(vecA, vecB);
    vecB = _mm512_min_epi32(vecA, vecB);
    vecB = _mm512_max_epu32(vecA, vecB);
    vecB = _mm512_min_epu32(vecA, vecB);
    return (int)((char *)&vecB)[0];
}

$ nvc -tp skylake op_avx.c 
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_max_epu32':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1111: undefined reference to `llvm.x86.avx512.pmaxu.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_max_epi64':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1133: undefined reference to `llvm.x86.avx512.pmaxs.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_max_epu64':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1155: undefined reference to `llvm.x86.avx512.pmaxu.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_min_epi32':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1324: undefined reference to `llvm.x86.avx512.pmins.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_min_epu32':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1346: undefined reference to `llvm.x86.avx512.pminu.d'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_min_epi64':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1368: undefined reference to `llvm.x86.avx512.pmins.q'
/cvmfs/soft.computecanada.ca/gentoo/2020/usr/x86_64-pc-linux-gnu/binutils-bin/2.33.1/ld: /tmp/nvclwCcHEoai04S.o: in function `_mm512_min_epu64':
/cvmfs/restricted.computecanada.ca/easybuild/software/2020/Core/nvhpc/22.1/Linux_x86_64/22.1/compilers/include/avx512fintrin.h:1390: undefined reference to `llvm.x86.avx512.pminu.q'

Bug reported to NVIDIA: https://forums.developer.nvidia.com/t/issue-with-some-avx512-intrinsics-min-max-ep-i-u-32-64/200868

[bosilca]

Awesome, they generate the AVX512 code, but fail at the linking stage due to missing intrinsics. Bottom line, yet another broken compiler, that we need to handle. Honestly I have 0 interest or incentive in fixing their mess, if anybody at Nvidia want's to fix this I will find the time to review their patch. Until then, people using nvc to compile Open MPI should disable any support for AVX*.