Skip to content
This repository was archived by the owner on Aug 11, 2023. It is now read-only.

MSVC STL version check #149

Merged
merged 7 commits into from
Nov 26, 2018
Merged

MSVC STL version check #149

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e54751e
Added check of MSVC STL compat
MathiasMagnus Oct 25, 2018
d744675
Fix argument passing to MSVC STL check
MathiasMagnus Oct 25, 2018
2ed1a98
Indentation fixes
MathiasMagnus Oct 25, 2018
3c161db
More indent fixes (hopefully last)
MathiasMagnus Oct 25, 2018
52b9b7b
Fix and simplify force-include for MSVC
MathiasMagnus Oct 26, 2018
90b39ff
Changed tab to spaces
MathiasMagnus Nov 15, 2018
083d030
Made changes along the lines of Duncan's suggestions
MathiasMagnus Nov 15, 2018
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions cmake/Modules/FindComputeCpp.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,57 @@ execute_process(COMMAND ${ComputeCpp_INFO_EXECUTABLE}
"--dump-device-compiler-flags"
OUTPUT_VARIABLE COMPUTECPP_DEVICE_COMPILER_FLAGS
RESULT_VARIABLE ComputeCpp_INFO_EXECUTABLE_RESULT OUTPUT_STRIP_TRAILING_WHITESPACE)
list(APPEND COMPUTECPP_DEVICE_COMPILER_FLAGS "-sycl-target ${COMPUTECPP_BITCODE}")
# convert to list before appending
# (VERBATIM arg passing to execute_process will go crazy over mixed string-list syntax)
string(REPLACE " " ";" COMPUTECPP_DEVICE_COMPILER_FLAGS ${COMPUTECPP_DEVICE_COMPILER_FLAGS})
list(APPEND COMPUTECPP_DEVICE_COMPILER_FLAGS} -sycl-target ${COMPUTECPP_BITCODE})

if(NOT ComputeCpp_INFO_EXECUTABLE_RESULT EQUAL "0")
message(FATAL_ERROR "compute++ flags - Error obtaining compute++ flags!")
else()
mark_as_advanced(COMPUTECPP_COMPILER_FLAGS)
endif()

# Check if the hosted STL of MSVC is compatible with ComputeCpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this whole code section is MSVC-specific, I think I'd be happier if it was part of an opt-in module (i.e. its own file). I feel like that would be a better reflection of the separation of concerns here - it could be called MSVCVersionCheck or similar, and that way users targetting '17 know that they have to deal with it (similarly, once the compiler is updated, they can remove it again).

Let me know what you think!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally okay with all this being moved to a file of its own. I'm not sure how many such changes are needed. There is a brief GCC-related test at the beginning, which for the sake of consistency might also be put to a different file... all in all, factoring out compiler-specific stuff to separate files and keeping FindComputeCpp.cmake to the bare minimum of detecting the library might be the best way to go. Also, checks about the executing runtime mentioned in #155 might also be put to a file of its own.

All of this might be the topic of another PR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer if we move it into its own file for now, provided it's not too difficult. Let me know if it looks like it will actually be too onerous.

You're right about the gcc/clang check. Revisiting it, I'm tempted to remove it, because who uses 4.8? 😝 More seriously, if you use old compilers like that, you will get compile issues. Though maybe we should move it to this new file while we still (ostensibly) support Ubuntu 14.04?

Decision time: if you feel like moving it too, please feel free, though I won't mark it as a requirement of this PR by any means. Thanks again!
if(MSVC)
set(ComputeCpp_STL_CHECK_SRC __STL_check)
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/${ComputeCpp_STL_CHECK_SRC}.cpp
"#include <ios>\n"
"int main() { return 0; }\n")
execute_process(
COMMAND ${ComputeCpp_DEVICE_COMPILER_EXECUTABLE}
${COMPUTECPP_DEVICE_COMPILER_FLAGS}
-isystem ${ComputeCpp_INCLUDE_DIRS}
-o ${ComputeCpp_STL_CHECK_SRC}.sycl
-c ${ComputeCpp_STL_CHECK_SRC}.cpp
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
RESULT_VARIABLE ComputeCpp_STL_CHECK_RESULT
ERROR_QUIET
OUTPUT_QUIET)
if(NOT ${ComputeCpp_STL_CHECK_RESULT} EQUAL 0)
# Try disabling compiler version checks
execute_process(
COMMAND ${ComputeCpp_DEVICE_COMPILER_EXECUTABLE}
${COMPUTECPP_DEVICE_COMPILER_FLAGS}
-D_ALLOW_COMPILER_AND_STL_VERSION_MISMATCH
-isystem ${ComputeCpp_INCLUDE_DIRS}
-o ${ComputeCpp_STL_CHECK_SRC}.cpp.sycl
-c ${ComputeCpp_STL_CHECK_SRC}.cpp
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
RESULT_VARIABLE ComputeCpp_STL_CHECK_RESULT
ERROR_QUIET
OUTPUT_QUIET)
if(NOT ${ComputeCpp_STL_CHECK_RESULT} EQUAL 0)
message(STATUS "Device compiler cannot consume hosted STL headers. Using any parts of the STL will likely result in device compiler errors.")
else()
message(STATUS "Device compiler does not meet certain STL version requirements. Disabling version checks and hoping for the best.")
list(APPEND COMPUTECPP_DEVICE_COMPILER_FLAGS -D_ALLOW_COMPILER_AND_STL_VERSION_MISMATCH)
endif()
endif()
file(REMOVE ${CMAKE_CURRENT_BINARY_DIR}/${ComputeCpp_STL_CHECK_SRC}.cpp
${CMAKE_CURRENT_BINARY_DIR}/${ComputeCpp_STL_CHECK_SRC}.cpp.sycl)
endif(MSVC)

find_package_handle_standard_args(ComputeCpp
REQUIRED_VARS ComputeCpp_ROOT_DIR
ComputeCpp_DEVICE_COMPILER_EXECUTABLE
Expand Down Expand Up @@ -375,15 +418,9 @@ function(__build_ir)
# Add both source and the sycl files to the VS solution.
target_sources(${SDK_BUILD_IR_TARGET} PUBLIC ${SDK_BUILD_IR_SOURCE} ${outputSyclFile})

# NOTE: The Visual Studio generators parse compile flags differently,
# hence the different argument syntax
if(CMAKE_GENERATOR MATCHES "Visual Studio")
set(forceIncludeFlags "/FI\"${includedFile}\" /TP")
else()
set(forceIncludeFlags /FI ${includedFile} /TP)
endif()
set(forceIncludeFlags "/FI ${includedFile} /TP")
else()
set(forceIncludeFlags "-include ${includedFile} -x c++")
set(forceIncludeFlags "-include ${includedFile} -x c++")
endif()

set_property(
Expand Down