Build error for complex floating point operations in GraphBLAS with clang-cl

[mmuetzel]

Describe the bug
Building GraphBLAS with clang-cl fails with errors like the following:

In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\FactoryKernels\GB_AxB__any_div_uint8.c:10:
In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\GB.h:56:
In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source/GB_ops.h:118:
D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\Factories\GB_ops_template.h(57,16): error: invalid argument type 'GxB_FC32_t' (aka 'struct _C_float_complex') to unary expression
        (*z) = GB_FC32_ainv (*x) ;
               ^~~~~~~~~~~~~~~~~
D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\Shared\GB_complex.h(249,30): note: expanded from macro 'GB_FC32_ainv'
    #define GB_FC32_ainv(x) (-(x))
                             ^~~~
In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\FactoryKernels\GB_AxB__any_div_uint8.c:10:
In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\GB.h:56:
In file included from D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source/GB_ops.h:118:
D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\Factories\GB_ops_template.h(493,16): error: invalid operands to binary expression ('const GxB_FC32_t' (aka 'const struct _C_float_complex') and 'const GxB_FC32_t')
        (*z) = GB_FC32_add (*x,*y) ;
               ^~~~~~~~~~~~~~~~~~~
D:\repo\SuiteSparse\SuiteSparse\GraphBLAS\Source\Shared\GB_complex.h(241,35): note: expanded from macro 'GB_FC32_add'
    #define GB_FC32_add(x,y) ((x) + (y))
                              ~~~ ^ ~~~

To Reproduce
Configure with the following using a MSVC build environment (might need to be adjusted depending on where the BLAS libraries are installed):

cmake.exe -G"Ninja" -DCMAKE_BUILD_TYPE="Release" -DCMAKE_PREFIX_PATH=C:/Users/Markus/anaconda3/envs/test/Library -DBLA_VENDOR="All" -DSUITESPARSE_ENABLE_PROJECTS="suitesparse_config;mongoose;amd;btf;camd;ccolamd;colamd;cholmod;cxsparse;ldl;klu;umfpack;paru;rbio;spqr;graphblas;lagraph" -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl -DSUITESPARSE_USE_CUDA=OFF ..

Expected behavior
GraphBLAS builds without errors.

Desktop (please complete the following information):

• OS: Windows
• compiler:

>clang-cl --version
clang version 16.0.5
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin

• Version: current head of dev2

[chengts95]

It is related to GB_COMPILER_MSC, but why using clang-cl here didn't set MSVC? I found that it may be due to GB_compiler.h, the clang flag is checked before MSVC. But what I want to do is to add an additional check #if GB_COMPILER_MSC || (__clang__ && defined( _MSC_VER )), which is on line 208 of GB_complex.h and line 571 of GB_ops.c.

[mmuetzel]

See #754 where the order of checks is already changed. Clang in MSVC-compatibility mode should be treated like MSVC in (almost) every aspect. Not just for this particular part of the code.

[chengts95]

Clang should not be treated as MSVC (since it has OpenMP 5.1 and there are many differences for the compilers). I can use clang grammars in clang-cl mode because the only problem is the std libs and headers of Windows SDK are used by both clang and clang-cl. I don't know if it will have a problem when using gnu-like clang on Windows, but logically we only need to include the proper headers and keep other clang Marcos unchanged.

[mmuetzel]

The availability of OpenMP and its version is checked independently on the used compiler.

Which "many differences" are you referring to?

[chengts95]

For C++, standard support will be very different. For C, language extensions are different, typedef float float4 __attribute__((ext_vector_type(4))); is still valid in clang-cl, but not valid in msvc. Clang wants to be compatible with MSVC, but it doesn't mean it becomes MSVC on Windows. If it wants to be MSVC, it makes nonsense to make __clang__ available in clang-cl.

[mmuetzel]

I haven't checked exactly how those macros are used in GraphBLAS. But when it comes to standard support, I agree that feature tests would probably be better than deciding based to the compiler vendor. The same goes for language extensions like symbol attributes.
The "main" difference between the msvc and the mingw targets is that they are using a different C (and C++) ABI. (Even the API is different when it comes to complex numbers.) In those respects, cl and clang-cl need to behave the same - or it wouldn't be possible, e.g., to link binaries compiled with cl to binaries compiled with clang-cl or vice versa...

Imho, it is a bit confusing that GraphBLAS has that extra step that translates preprocessor macros that would clearly indicate what would need to be distinguished here to other preprocessor macros that are based on compiler vendor names. But I guess that is a different issue to the one that is reported here.

I'll try and come up with feature tests for some of the decisions that are currently based on vendor names. But imho that doesn't need to hold back a fix for this issue.

[mmuetzel]

I pushed an additional commit to #754 that adds a feature test for how complex floating point numbers are handled by the used compiler.
Eventually similar feature tests could be added to check for support of atomic captures that are currently also guarded by the vendor. (And other checks where that would make sense.)

[DrTimothyAldenDavis]

This is fixed now, in the dev2 branch, correct?

[mmuetzel]

Yes, the original issue is fixed and covered in CI.
I'm not sure if it would make sense to add more feature tests to check which code branch compilers should select, e.g., for atomic exchange...