Debugging on Windows and cross-device #151
Conversation
CMakeLists.txt
Outdated
| ) | ||
| if (MSVC) | ||
| target_link_libraries (unittest | ||
| qrack |
There was a problem hiding this comment.
Can we pull these out to a single variable specified somewhere so we don't have to duplicate these for each call?
CMakeLists.txt
Outdated
| target_compile_options (unittest PUBLIC -O3 -std=c++11 -Wall -Werror ${TEST_COMPILE_OPTS} -DCATCH_CONFIG_FAST_COMPILE) | ||
| target_compile_options (benchmarks PUBLIC -O3 -std=c++11 -Wall -Werror ${TEST_COMPILE_OPTS} -DCATCH_CONFIG_FAST_COMPILE) | ||
| if (MSVC) | ||
| target_compile_options (qrack PUBLIC -std=c++11 -Wall -fPIC ${QRACK_COMPILE_OPTS} -DCATCH_CONFIG_FAST_COMPILE) |
There was a problem hiding this comment.
Also, let's pull the duplication out of these (and related) lines so that they're not redundantly specified for each executable linked.
cmake/OpenCL.cmake
Outdated
| @@ -1,30 +1,19 @@ | |||
| option (ENABLE_OPENCL "Use OpenCL optimizations" ON) | |||
|
|
|||
| set (OPENCL_AMDSDK /opt/AMDAPPSDK-3.0 CACHE PATH "Installation path for the installed AMD OpenCL SDK, if used") | |||
| set (OPENCL_NVIDIASDK "C:/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v10.0" "Installation path for the installed NVIDIA CUDA Toolkit, if used") | |||
There was a problem hiding this comment.
Tag by OS, namely, OPENCL_WIN_NVIDIASDK.
There was a problem hiding this comment.
It's not necessary, since find_package() will handle this if it's installed, but I'll leave a conditional check for AMD on VMWare.
| CACHE STRING "AMD OpenCL SDK Compilation Option Requirements") | ||
| message ("OpenCL support found in the AMD SDK") | ||
| endif () | ||
| find_package (OpenCL) |
There was a problem hiding this comment.
This doesn't work if you manually installed the SDK (as I had to do on VMWare), as well as requiring a PACKAGE_CMake.txt file to exist in the search path.
src/common/parallel_for.cpp
Outdated
| @@ -136,7 +138,10 @@ void ParallelFor::par_for_mask( | |||
| } | |||
|
|
|||
| /* Pre-calculate the masks to simplify the increment function later. */ | |||
| bitCapInt masks[maskLen][2]; | |||
| bitCapInt** masks = new bitCapInt*[maskLen]; | |||
There was a problem hiding this comment.
Did the indentation change here?
src/common/parallel_for.cpp
Outdated
| @@ -161,6 +166,11 @@ void ParallelFor::par_for_mask( | |||
|
|
|||
| par_for_inc(begin, (end - begin) >> maskLen, incFn, fn); | |||
| } | |||
|
|
|||
| for (int i = 0; i < maskLen; i++) { | |||
There was a problem hiding this comment.
Did the indentation change here?
| posix_memalign((void**)&nrmArray, ALIGN_SIZE, nrmVecAlignSize); | ||
| #elif defined(_WIN32) && !defined(__CYGWIN__) |
There was a problem hiding this comment.
How necessary are the CYGWIN references? If it's strictly gated on _WIN32, will the right thing happen?
There was a problem hiding this comment.
If it's strictly gated on _WIN32, will the right thing happen?
No. _WIN32 will be defined, if building in CYGWIN, but the desired build behavior should be the same as for Linux, in that case.
| QEngineOCLPtr copyPtr = std::make_shared<QEngineOCL>( | ||
| qubitCount, 0, rand_generator, complex(ONE_R1, ZERO_R1), doNormalize, randGlobalPhase, useHostRam, deviceID); | ||
|
|
||
| clFinish(); | ||
| copyPtr->clFinish(); |
There was a problem hiding this comment.
Looks like a bunch of these got transitioned from 4 space indent to one tab indent. Probably need to re-run the style tool.
There was a problem hiding this comment.
Yes, Visual Studio kept telling me that it was applying our clang-format definition, but it apparently screwed it up. I ran make format on Linux, just now.
| @@ -48,17 +48,30 @@ void log(QInterfacePtr p) { std::cout << std::endl << std::showpoint << p << std | |||
|
|
|||
| unsigned char* cl_alloc(size_t ucharCount) | |||
There was a problem hiding this comment.
Still seems weird to me to have these calls in the test/tests.cpp code, since don't we have the same functionality elsewhere?
In an effort to test this on a Windows machine with an AMD, I debugged our Windows build and unit tests. This passes all tests on the Windows partition of my primary development machine, except for the noted issue with
ApproxCompare(). Windows 10 has OpenCL drivers for all of my devices, so this gave me an opportunity to test and debug a configuration with 3 devices on 2 platforms.