kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
413 stars 69 forks source link

MSVC 19.x is not working #276

Open sunavlis opened 1 year ago

sunavlis commented 1 year ago

The mdspan library doesn't compile when using it in Visual Studio 2022 and MSVC 19.35.

With the following cMake and example source results in a compilation error:

CMakeLists.txt.

cmake_minimum_required(VERSION 3.13 FATAL_ERROR)

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED Yes)

project(mdspanTest
    VERSION 0.0.1
)

set(MDSPAN_CXX_STANDARD ${CMAKE_CXX_STANDARD} CACHE INTERNAL "Use CMAKE_CXX_STANDARD .")
include(FetchContent)
FetchContent_Declare(mdspan
    GIT_REPOSITORY https://github.com/kokkos/mdspan.git
    GIT_TAG mdspan-0.6.0
)
FetchContent_GetProperties(mdspan)
if(NOT mdspan_POPULATED)
    FetchContent_Populate(mdspan)
    add_subdirectory(${mdspan_SOURCE_DIR} ${mdspan_BINARY_DIR} EXCLUDE_FROM_ALL)
endif()

add_executable(${PROJECT_NAME}
  main.cpp
)

target_link_libraries(${PROJECT_NAME} 
    std::mdspan
)

main.cpp

#include <cstddef>
#include <iostream>
#include <experimental/mdspan>
#include <vector>

int main()
{
    std::vector v{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 };
    auto ms2 = std::experimental::mdspan(v.data(), 2, 6);
}

Error

  [1/2] Building CXX object CMakeFiles\mdspanTest.dir\main.cpp.obj
  FAILED: CMakeFiles/mdspanTest.dir/main.cpp.obj 
  C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1435~1.322\bin\Hostx64\x64\cl.exe  /nologo /TP  -IC:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include /DWIN32 /D_WINDOWS /W3 /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1 -std:c++17 /showIncludes /FoCMakeFiles\mdspanTest.dir\main.cpp.obj /FdCMakeFiles\mdspanTest.dir\ /FS -c C:\Projects\Mdspan_Test\main.cpp
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(375): warning C4346: 'Extents::rank': dependent name is not a type
  C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(375): note: prefix with 'typename' to indicate a type
  C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(475): note: see reference to class template instantiation 'std::experimental::layout_stride::mapping<Extents>' being compiled
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(375): error C2059: syntax error: '...'
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(382): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(382): error C2062: type 'unknown-type' unexpected
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(384): error C2334: unexpected token(s) preceding '{'; skipping apparent function body
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(395): error C2270: 'is_exhaustive': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_stride.hpp(406): error C2270: 'stride': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(159): warning C4346: 'std::experimental::layout_right::mapping<Extents>::std::experimental::layout_right::mapping<Extents>::extents_type::rank': dependent name is not a type
  C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(159): note: prefix with 'typename' to indicate a type
  C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(220): note: see reference to class template instantiation 'std::experimental::layout_right::mapping<Extents>' being compiled
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(159): error C2059: syntax error: '...'
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(168): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(168): error C2062: type 'unknown-type' unexpected
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(170): error C2334: unexpected token(s) preceding '{'; skipping apparent function body
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(177): error C2270: 'is_unique': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(178): error C2270: 'is_exhaustive': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(179): error C2270: 'is_strided': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\layout_right.hpp(186): error C2270: 'stride': modifiers not allowed on nonmember functions
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\mdspan.hpp(110): error C2059: syntax error: '...'
  C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits/mdspan.hpp(372): note: see reference to class template instantiation 'std::experimental::mdspan<ElementType,Extents,LayoutPolicy,AccessorPolicy>' being compiled
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\mdspan.hpp(119): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\mdspan.hpp(119): error C2062: type 'unknown-type' unexpected
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\mdspan.hpp(123): error C2334: unexpected token(s) preceding ':'; skipping apparent function body
C:\Projects\Mdspan_Test\out\build\x64-Debug\_deps\mdspan-src\include\experimental\__p0009_bits\mdspan.hpp(26): fatal error C1075: '{': no matching token found
  ninja: build stopped: subcommand failed.

Or here in the online demo: https://godbolt.org/z/W9bhGxzqf

Any idea where the error is from? And how it can be fixed?

Thanks, Silvan

sunavlis commented 1 year ago

I just found a hint that msvc may contains a compiler bug for fold expression AND: https://stackoverflow.com/questions/56978499/compiler-error-with-a-fold-expression-in-enable-if-t

mhoemmen commented 1 year ago

@sunavlis Thanks for reporting! : - ) This example works on the latest MSVC on Compiler Explorer: https://godbolt.org/z/vxT3vKr6x , when I use /std:c++latest or /std:c++20, but it gives a build error like the one you showed with /std:c++17.

Are you able to build with C++20 or later, or do you actually need C++17 to work for now?

sunavlis commented 1 year ago

@mhoemmen thanks for your answer. Unfortunately it is true, that our project uses C++17. And at the moment it's not planed to migrate to a newer version of C++.

I did a few changes in the sources and used std::conjunction instead of the _MDSPAN_FOLD_AND. (https://github.com/sunavlis/mdspan/commit/46b5b25be7e26e4ce08f38b99474272220009fb9) but now I'm stuck when trying to fix the extents method at: https://github.com/sunavlis/mdspan/blob/stable/include/experimental/__p0009_bits/extents.hpp#L494

mhoemmen commented 1 year ago

@sunavlis Thanks for working on a fix!

but now I'm stuck when trying to fix the extents method....

Is the issue with the fold expression inside MDSPAN_CONDITIONAL_EXPLICIT? The MDSPAN_CONDITIONAL_EXPLICIT macro should be empty in C++17 mode. Nevertheless, could you try just deleting lines 506-510? If that works, we can protect those lines with #if defined(_MSC_VER) && defined(_MSVC_LANG) (as MSVC defines __cplusplus to 199711L unless /Zc:__cplusplus is set, no matter the language version passed to /std).

sunavlis commented 1 year ago

No the issue isn't in MDSPAN_CONDITIONAL_EXPLICIT. This macro is empty as expected.

The issue is in the __check_compatible_extents function or its parameters used by the macro MDSPAN_TEMPLATE_REQUIRES.

As soon as std::integer_sequence<size_t, Extents...>{} is used, I got the following errors:

C:\Projects\git\github\sunavlis\mdspan\include\experimental\__p0009_bits\extents.hpp(510): error C3546: '...': there are no parameter packs available to expand
  C:\Projects\git\github\sunavlis\mdspan\tests\test_extents.cpp(335): note: see reference to class template instantiation 'Kokkos::extents<size_t,18446744073709551615,18446744073709551615>' being compiled
C:\Projects\git\github\sunavlis\mdspan\include\experimental\__p0009_bits\extents.hpp(512): error C3520: 'Extents': parameter pack must be expanded in this context
  C:\Projects\git\github\sunavlis\mdspan\include\experimental\__p0009_bits\extents.hpp(51): note: see reference to function template instantiation 'std::integral_constant<bool,> Kokkos::detail::__check_compatible_extents(std::integral_constant<bool,true>,std::integer_sequence<size_t,_Vals...>,std::integer_sequence<size_t,OtherExtents...>) noexcept' being compiled

For testing, I changed the __check_compatible_extents implementation and a version without std::integer_sequence<size_t, Extents...>{} compiles. I'm not sure whats happen here...

mhoemmen commented 1 year ago

Thanks for working on this! : - ) I've seen some suggestions to include extra parentheses around (and perhaps also inside) fold expressions.

This week is the C++ Standard Committee plenary meeting, but we should have some more time to look at this in a few days.

sunavlis commented 1 year ago

Thanks for your reply. In the meantime I found additional interesting facts:

I focused on the differences in MSVC when building with C++17 (/std:c++17) and C++20 (/std:c++20). It seams, that the default value of a compiler flag has changed between this two versions: https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170

In C++20 the flag is set to /permissive- which enforce the standards conformance where in C++17 it is not set. When the flag is set, the existing code on the stable branch builds also with the current version of MSVC.

Changes are here: https://github.com/sunavlis/mdspan/commit/decc8f65d867445095b0152258c13fc4afda6b9a

I think it would be nice to have source code which works with many compilers and flags as possible, but with this pragmatic change the library could be used with MSVC with really small changes in the library it self.

What do you think? I could create a merge request to push the changes back into the stable branch.

crtrott commented 1 year ago

Yeah we could add that flag, but I also can check if I find another workaround. I am coming back from two weeks of travel including ISO C++ committee today, and might be able to look a bit more into this. That said I am generally all for "make your compiler behave in a standard compliant way" lol