Does not compile using vs2019 about graphblas-feedstock HOT 27 CLOSED

That's for static libraries. Not DLLs

from graphblas-feedstock.

My suggestion (-DGxB_NO_FIRST_FC32) wouldn't have any effect on the functionality of Octave, if it were to use it via conda-forge. It would at most make that particular operation a little slower, but maybe none at all. I know that GraphBLAS can be used in Octave, via its MATLAB/Octave interface, but I don't think GraphBLAS is a core dependency inside Octave itself.

So this change would be pretty safe I think.

from graphblas-feedstock.

Thanks @DrTimothyAldenDavis. I will run these experiments. It's easy to conditionally apply a patch so we can be surgical, but first I'll try disabling fc32 (and fc64 if necessary) everywhere to see what happens.

from graphblas-feedstock.

>=1928 is probably a safer bet

On second thought, since this is about pessimisation to avoid a bug, and we don't know exactly when it was introduced (and neither should you spend time IMO to investigate), probably >=1920 after all. I'd also add <1930, as long as we haven't tested VS2022 yet

from graphblas-feedstock.

Thanks. I've got a draft in progress, which disables these 2 methods for _MSC_VER between 1920 to 1929.

from graphblas-feedstock.

Closing. v7.3.1 is released and works with vs2019 w/o a patch. Thanks @DrTimothyAldenDavis!

from graphblas-feedstock.

Thanks everyone for the help here! 🙏

from graphblas-feedstock.

cc @h-vetinari

from graphblas-feedstock.

@eriknw The fact that the error happens in fc32 is highly suspicious. In fact, why is it compiling that file at all? I thought we disabled all complex number handling for Windows.

from graphblas-feedstock.

GraphBLAS compiles with support for complex numbers on Windows, but we don't support complex in python-suitesparse-graphblas because of limitations of cffi. We should try to not remove support for complex numbers on Windows b/c GraphBLAS is used by more than Python. Note that there are known limitations with complex numbers in Windows, such as asinh behaves improperly.

from graphblas-feedstock.

GraphBLAS compiles with support for complex numbers on Windows, but we don't support complex in python-suitesparse-graphblas because of limitations of cffi.

Thinking we might want to disable complex numbers on windows in any case...? It's possible to do things with shim types (msvc does support C99 complex, but not with standard types... 🤦), but that's generally a major PITA, especially if those types end up in the interface.

So it just shows up "the most" in FFI, but any consuming library will have the same issues.

But I'm not sure I understand how the move to vs2019 broke things... Since the log basically shows an ICE (internal compiler error; those are fixed by MS with high priority), I think the best approach might just be to raise a bug on the VS community tracker.

Sidenote: going back to vs2017 (in conda_build_condig.yaml) would be possible short-term, but not long-term (unless also pinning llvm-openmp<14, which is not ecosystem-compatible), because LLVM 14+ requires vs2019.

from graphblas-feedstock.

Sidenote: going back to vs2017 (in conda_build_condig.yaml) would be possible short-term, but not long-term (unless also pinning llvm-openmp<14, which is not ecosystem-compatible), because LLVM 14+ requires vs2019.

This is wrong. As long as DLLs are used, compiling and linking graphblas with llvm-openmp 14+ using vs2017 should be possible.

from graphblas-feedstock.

This is wrong. As long as DLLs are used, compiling and linking graphblas with llvm-openmp 14+ using vs2017 should be possible.

I was referring to this statement in the docs:

However, you must link by using a toolset at least as recent as the most recent binary in your app.

I understood that as having to use vc142/vs2019 to be able to consume the openmp DLL built by vc142/vs2019, but if it works after all - happy to hear!

from graphblas-feedstock.

Thanks for helping all.

Perhaps this issue is related: https://developercommunity.visualstudio.com/t/fatal-error-C1001:-Internal-compiler-err/1390698

In the meantime, you can work-around this issue by disabling the phase that triggers this problem, by using the /d2ssa-cse- switch.

How do we try the "/d2ssa-cse- switch"? Do I add something to the cmake command in bld.bat? I'm a novice at compiling on Windows, so I need something I can copy/paste.

from graphblas-feedstock.

Thinking we might want to disable complex numbers on windows in any case...?

That doesn't sound unreasonable. @DrTimothyAldenDavis, is there a way to disable complex on Windows (I don't think there currently is, right?)? Do you know if anybody actually uses complex on Windows? It sounds like we can continue using vs2017 for now, so it's not pressing.

from graphblas-feedstock.

Disable complex on Windows would break MATLAB C=A*B, which uses GraphBLAS.

from graphblas-feedstock.

Disable complex on Windows would break MATLAB C=A*B, which uses GraphBLAS.

But MATLAB is not built on conda-forge packages. What I was talking about was not to change anything upstream¹, but to modify the packaging recipe here to disable complex on windows just within conda-forge.

from graphblas-feedstock.

Got it. Then I have an easy solution.

I have a mechanism for disabling any data type, operator, semiring, monoid, etc. I implemented this a while back as a way to save compile time and file size of the resulting compiled library.

A disabled type / operator /etc is still available, it's just slower because it uses what I call a "generic" method, with function pointers for the operators for example, and memcpy for the assignments. If you use C=A*B for example, in the PLUS_TIMES_FP64 semiring, but A is FP32, then I have to typecast. I don't have such a kernel, so I call my generic method.

The GB_control.h file contains the possible control settings (it's long ...!). See:
https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/stable/Source/GB_control.h

What you could do is to compile all of GraphBLAS with -DGxB_NO_FC32 for example, and also -DGxB_NO_FC64. See: https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/97510b55fba589e6ea315fe433237633057e7048/Source/GB_control.h#L176
You can enable that setting by uncommenting line 176 and 177, or better yet by compiling all of GraphBLAS with

-DGxB_NO_FC32 -DGxB_NO_FC64

That would force all complex-type operators to go the "generic" route. With this setting, I would not compile the code that is causing the bug, but you would also find all complex types to be slow.

I think the compiler bug is triggered by this code:
https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/stable/Source/Generated2/GB_binop__first_fc32.c

Take a look at this line:
https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/97510b55fba589e6ea315fe433237633057e7048/Source/Generated2/GB_binop__first_fc32.c#L119

You could be even more selective, if you like. If this is the only file that triggers the compiler bug, you can compile all of GraphBLAS with this:

-DGxB_NO_FIRST_FC32

Then, only 5 files in Source/Generated2 get disabled:

 $ ack -l GxB_NO_FIRST_FC32
GB_control.h
Generated2/GB_AxB__any_first_fc32.c
Generated2/GB_AxB__plus_first_fc32.c
Generated2/GB_AxB__times_first_fc32.c
Generated2/GB_binop__first_fc32.c
Generated2/GB_red__first_fc32.c

That is, three semrings: GxB_ANY_FIRST_FC32, PLUS_FIRST_FC32, TIMES_FIRST_FC32,
the binary ops (eWiseAdd, eWiseMult, and so on) for FIRST_FC32, and the reduction operator (matrix or vector to scalar).
Note that the reduction using the FIRST operator is rather unusual. It's not part of the normal
reduction to scalar by GrB_reduce. I use these also for building a matrix with GrB_Matrix_build, which can use the FIRST
operator.

The FIRST operator is surprisingly useful in GraphBLAS, so it's a shame to make these 5 methods Generic if you don't have to. The advantage of compiling all of GraphBLAS with -DGxB_NO_FIRST_FC32 is that there are no changes need to the source code, at all. Just toss that flag into all of GraphBLAS.

As an alternative, if it's possible in CMake, you could compile just the one offending file with -DGxB_NO_FIRST_FC32.

Or if you want to edit a file (maybe with a script?), you could revise the file
https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/97510b55fba589e6ea315fe433237633057e7048/Source/Generated2/GB_binop__first_fc32.c#L121
and change these lines:

// disable this operator and use the generic case if these conditions hold
#define GB_DISABLE \
    (GxB_NO_FIRST || GxB_NO_FC32 || GxB_NO_FIRST_FC32)

to

// disable this operator and use the generic case if these conditions hold
#define GB_DISABLE \
    (1)

I don't have a method to manually edit the files in the Source/Generated* folders. These are all auto-generated via MATLAB and m4 scripts (see Source/codegen*.m). Yea, I know, it's pretty crazy :-)

But this change would be the most surgical, affecting just a single file, not all 5.

I would suggest first just trying -DGxB_NO_FIRST_FC32 and see if that fixes the problem.

from graphblas-feedstock.

You could also make this dependent on the compiler version. If you're using vs2019 then add -DGxB_NO_FIRST_FC32, but leave that option out for all other cases.

from graphblas-feedstock.

Disable complex on Windows would break MATLAB C=A*B, which uses GraphBLAS.

But MATLAB is not built on conda-forge packages. What I was talking about was not to change anything upstream1, but to modify the packaging recipe here to disable complex on windows just within conda-forge.

Footnotes

1. except perhaps a switch we could use, so we don't have to monkeypatch. ↩

That said, we do ship Octave in conda-forge. Though Idk if we are publishing GraphBLAS packages usable from Octave.

from graphblas-feedstock.

Let me know if -DGxB_NO_FIRST_FC32 fixes the problem for vs2019. If so, I'll post it as an issue in the main GraphBLAS and SuiteSparse repos, and I'll add a note in the GraphBLAS documentation.

from graphblas-feedstock.

Compiled 🎉! I added the patch you describe to disable first_fc32 and second_fc32 on Windows. See: #49

from graphblas-feedstock.

Awesome! For GraphBLAS v7.3.1, I can be more selective, and modify my source code generator so that it more selectively disables just the functions that are failing. I would need a little help, only to know the value of _MSC_VER that is vs2019. I already query the compiler and version here:

https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/9248cabd439c2578f08b51b96c657d1d25e1c99e/Source/GB_compiler.h#L120

If I knew the _MSC_VER (or ranges) that has the compiler bug, I could add a special disable flag that would turn on the GB_DISABLE flag for just these 2 files:

Generated2/GB_binop__first_fc32.c
Generated2/GB_binop__second_fc32.c

Those are the eWise operators that use the FIRST_FC32 and SECOND_FC32 operators. The use of eWiseAdd, eWiseMult, C=AD and C=DB where D is diagonal, would be disabled, but those are likely rare.

Much more commom would be GrB_Matrix_build for FC32 matrices, when the "dup" operator is set to GxB_IGNORE_DUP (which means duplicates are ignored, and I take the last tuple seen. This is a SuiteSparse:GraphbLAS extension but it's really important). That method would rely on the file:

Generated2/GB_red__second_fc32.c

which doesn't trigger the compiler bug, and which is very important for performance when working with single-precision complex matrices. So that file should really not be disabled, if at all possible.

from graphblas-feedstock.

If I knew the _MSC_VER (or ranges) that has the compiler bug, [...]

The version number I see is 19.29.30139. I guess this is the version returned by cl.exe. I don't actually understand VS versions (see #45 (comment)). Good luck!

from graphblas-feedstock.

If I knew the _MSC_VER (or ranges) that has the compiler bug, [...]

The version number I see is 19.29.30139. I guess this is the version returned by cl.exe. I don't actually understand VS versions (see #45 (comment)). Good luck!

_MSC_VER >= 1920 gets you all of the VS2019 series. The earlier versions aren't much in use though anymore, and might not work. >=1928 is probably a safer bet, that's when C11 support arrived. The last version of the VS2019 series (i.e. the one in public CI; realistically the only one that actually gets tested) would be >=1929.

The comment linked by @eriknw that I made in the other PR has more details.

from graphblas-feedstock.

Can you give v7.3.1.beta1 a try, without any work-around on your side, to make sure this is fixed in v7.3.1?

from graphblas-feedstock.

v7.3.1.beta1 works w/o a patch: #50

from graphblas-feedstock.