[Windows] Bug fixes for MSVC #1042

willyd · 2017-08-07T19:09:24Z

file_store_handler.cc: mkdir only accepts one argument and requires inclusion of <direct.h>
math.h: macro workaround does not work for integerIsPowerOf2 when prefixed with math namespace.
GpuBitonicSort.cuh: use std::integral_constant since nvcc ignores constexpr with MSVC (fixes Compile Error on Windows #997)
pool_op_cudnn.cu: undefine IN and OUT macros defined in minwindef.h
logging.cc: Prefix glog logging levels with name since MSVC cannot use the abbreviated macros

facebook-github-bot · 2017-08-08T01:20:23Z

@willyd updated the pull request - view changes

Yangqing · 2017-08-08T23:42:01Z

Thanks! We had some glitches in the pull request import process, and it's solved now. Will merge.

facebook-github-bot · 2017-08-08T23:42:13Z

@willyd updated the pull request - view changes

facebook-github-bot · 2017-08-08T23:42:55Z

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Orvid · 2017-08-08T23:58:58Z

caffe2/distributed/file_store_handler.cc

@@ -6,6 +6,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/stat.h>
+#if defined(_MSC_VER)
+#include <direct.h>


This is already being included further down on line 19.

Kindly rebase and remove this?

Orvid · 2017-08-09T00:03:24Z

caffe2/utils/GpuBitonicSort.cuh

@@ -39,9 +39,11 @@ __device__ inline void bitonicSort(K* keys,
  // Assume the sort is taking place in shared memory
  // static_assert(Power2SortSize * (sizeof(K) + sizeof(V)) < 32768,
  //               "sort data too large (>32768 bytes)");
-  static_assert(math::integerIsPowerOf2(Power2SortSize),
+  static_assert(math::integerIsPowerOf2(
+                std::integral_constant<int, Power2SortSize>()),


Shouldn't this be std::integral_constant<int, Power2SortSize>()::value?

Yep, this seems to be needed, otherwise build errors.

Yangqing · 2017-08-09T04:33:20Z

caffe2/utils/GpuBitonicSort.cuh

                "sort size must be power of 2");
-  static_assert(math::integerIsPowerOf2(ThreadsPerBlock),
+  static_assert(math::integerIsPowerOf2(
+                std::integral_constant<int, ThreadsPerBlock>()),


Here as well

Yangqing · 2017-08-09T04:33:25Z

caffe2/utils/GpuBitonicSort.cuh

@@ -107,7 +109,8 @@ __device__ inline void warpBitonicSort(K* keys,
  // Smaller sorts should use a warp shuffle sort
  static_assert(Power2SortSize > kWarpSize,
                "sort not large enough");
-  static_assert(math::integerIsPowerOf2(Power2SortSize),
+  static_assert(math::integerIsPowerOf2(
+                std::integral_constant<int, Power2SortSize>()),


Yangqing

(kindly see comments above)

facebook-github-bot · 2017-08-09T13:12:25Z

@willyd updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-08-09T14:09:03Z

@willyd updated the pull request - view changes - changes since last import

willyd · 2017-08-09T14:13:09Z

@Yangqing Thanks for the review. I rebased and removed the requested #include. Plus I fixed a few other issues. One that you might want to look into is the missing CMakeLists.txt file in contrib/opengl.

Yangqing · 2017-08-09T16:48:13Z

Thanks @willyd - yeah, we traced the issue back to our sync script ignoring the CMakeLists.txt files during syncing, and we are fixing it. Let me pull the PR and land it if all checks out.

facebook-github-bot · 2017-08-09T16:48:29Z

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-08-10T00:32:31Z

@willyd updated the pull request - view changes - changes since last import

Yangqing · 2017-08-10T00:33:21Z

@willyd do you mind rebase to master, and also address the std::integral_constant<int, Power2SortSize>()::value comment?

- file_store_handler.cc: mkdir only accepts one argument and requires inclusion of <direct.h> - math.h: macro workaround does not work for integerIsPowerOf2 when prefixed with `math` namespace. - GpuBitonicSort.cuh: use std::integral_constant since nvcc ignores constexpr with MSVC - pool_op_cudnn.cu: undefine IN and OUT macros defined in minwindef.h - logging.cc: Prefix glog logging levels with name since MSVC cannot use the abbreviated macros

…ion problems with MSVC

facebook-github-bot · 2017-08-10T17:04:17Z

@willyd updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-08-10T17:24:50Z

@willyd updated the pull request - view changes - changes since last import

willyd · 2017-08-10T18:14:15Z

@Yangqing Rebase done. Also addressed the ::value comment.

facebook-github-bot · 2017-08-11T02:22:46Z

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

harouwu · 2017-08-17T22:55:26Z

@Yangqing @willyd what is the update on this PR? If it looks good, @willyd would you rebase and we are ready to go?

willyd · 2017-08-18T15:40:37Z

@harouwu I rebased on master last time @Yangqing asked. Since then 69 new commits have been pushed to master, but none of them have conflicting changes as per github. I don't see the need for another rebase, can you please elaborate on why I would need to rebase again?

I am basically waiting for his feedback or for him to merge the PR.

Callum17 · 2017-08-28T05:03:35Z

Did a fresh build from latest master commit @ 799d7cc. Merged with this pull request @ 6784956. Successful compilation with the following change:

Minor issue where MSVC complains about type narrowing conversions in {base}\caffe2\caffe2\layer_norm_op.cu at lines 264 and 294.

264: gscratch_.Resize(std::vector<size_t>{left, right});
should be
264: gscratch_.Resize(std::vector<size_t>{static_cast<size_t>(left), static_cast<size_t>(right)});

294: dstdev_.Resize(vector<size_t>{left, 1});
should be
294: dstdev_.Resize(vector<size_t>{static_cast<size_t>(left), 1});

Looks promising after successful convnet_benchmark runs for AlexNet, OverFeat, Inception.

VGGA model crashes. MLP returns with error:

Traceback (most recent call last):
File "D:\python27\Lib\runpy.py", line 174, in _run_module_as_main
"main", fname, loader, pkg_name)
File "D:\python27\Lib\runpy.py", line 72, in _run_code
exec code in run_globals
File "C:\Users\Callum\Documents\GitHub\caffe2\build\caffe2\experiments\python\convnet_benchmarks.py", line 685, in
Benchmark(model_map[args.model], args)
File "C:\Users\Callum\Documents\GitHub\caffe2\build\caffe2\experiments\python\convnet_benchmarks.py", line 573, in Benchmark
AddParameterUpdate(model)
File "C:\Users\Callum\Documents\GitHub\caffe2\build\caffe2\experiments\python\convnet_benchmarks.py", line 534, in AddParameterUpdate
param_grad = model.param_to_grad[param]
KeyError: BlobReference("fc_1_0_w")

- Narrowing conversions in image_input_op.h, batch_matmul_op.cc and layer_norm_op.cu - Removed ThreadPool.cc from Windows build since it requires pthread

facebook-github-bot · 2017-08-31T13:08:58Z

@willyd updated the pull request - view changes - changes since last import

willyd · 2017-08-31T13:13:41Z

Gentle reminder for @Yangqing.

I just updated the PR to latest master and fixed the issues mentioned by @Callum17 and some others. Should I squash the commits into a single one?

pengfei666 · 2017-09-04T06:45:08Z

@willyd This PR is very useful, I have compiled successfully, thanks. Hope it could be merged soon.

…c CRT (/MT) for static build.

facebook-github-bot · 2017-09-12T18:55:18Z

@willyd updated the pull request - view changes - changes since last import

willyd · 2017-09-12T19:06:04Z

I just added a few more commits to support building with a dynamic runtime library and to support building with vcpkg. I also submitted a PR to vcpkg. If anyone is interested in testing it, it would be much appreciated.

Commit a7ece7c should fix #403.

willyd · 2017-09-20T00:44:26Z

caffe2 (CPU and C++ only) is now available on windows via vcpkg. Installing should be as simple as: vcpkg install caffe2 --triplet x64-windows or vcpkg install caffe2 --triplet x64-windows-static.

to compile with MSVC

hughsando · 2017-10-23T15:13:30Z

Not sure if this is the best place to mention this - and it might be covered by the PRs here - some of the other issues I had were.
In caffe2/perfkernels/embedding_lookup_avx2.cc, the CAFFE2_ALIGNED(64) macro should come before the variable declarations, and _mm_prefetch needs a type-cast for the point arg in msvc. This is auto-generated so the fixes will be elsewhere.

aralph · 2017-11-07T13:41:22Z

CMakeLists.txt

@@ -56,6 +56,14 @@ option(USE_ROCKSDB "Use RocksDB" ON)
 option(USE_SNPE "Use Qualcomm's SNPE library" OFF)
 option(USE_THREADS "Use Threads" ON)
 option(USE_ZMQ "Use ZMQ" OFF)
+if(MSVC)
+  if(BUILD_BUILD_SHARED_LIBS)


Shouldn't the variable be 'BUILD_SHARED_LIBS'?

@aralph You are right! Thanks for pointing out. I must'nt have had enough coffee when I wrote this. When I get back to caffe2 on Windows I will update the PR.

jimmyoic · 2017-12-31T21:37:20Z

Hi,
Did anyone else get the problems about link error?
I follow the post (https://research.wmz.ninja/articles/2017/05/build-caffe2-on-windows-10-gpu.html), but I kept getting the error message ( undefined external symbol...)

I tried add the dependencies of cuda library in VS 2015, but it didn't work.

orionr · 2018-09-27T15:34:31Z

Thank you for your contribution! PyTorch and Caffe2 are now officially merged with the PyTorch 1.0 preview release. Please submit on https://github.com/pytorch/pytorch if this is still an issue over there.

facebook-github-bot added the CLA Signed label Aug 7, 2017

willyd mentioned this pull request Aug 7, 2017

Miscellaneous fixes for compatibility with MSVC #878

Closed

Orvid reviewed Aug 9, 2017

View reviewed changes

Yangqing reviewed Aug 9, 2017

View reviewed changes

willyd force-pushed the msvc-fixes branch from 555e4af to 4487674 Compare August 9, 2017 13:12

willyd added 5 commits August 10, 2017 12:40

[Windows] Fixed narrowing conversion error in lengths_top_k_op.cc

c9e6dfb

Disabling opengl build since CMakeLists.txt is missing

959512e

Moved cuDNN version check outside CUDNN_ENFORCE to avoid macro expans…

c01d606

…ion problems with MSVC

Replaced std::integral_constant<>() by std::integral_constant<>::value

9895067

willyd force-pushed the msvc-fixes branch from 511ad8e to 9895067 Compare August 10, 2017 17:04

Roll back opengl related change

6784956

willyd added 2 commits August 30, 2017 15:57

Merge branch 'master' into msvc-fixes

4f117d9

[Windows] Fixed issues with MSVC:

fadf7e0

- Narrowing conversions in image_input_op.h, batch_matmul_op.cc and layer_norm_op.cu - Removed ThreadPool.cc from Windows build since it requires pthread

willyd added 3 commits September 11, 2017 13:37

Allow building caffe2 with dynamic CRT (/MD). Still defaults to stati…

a7ece7c

…c CRT (/MT) for static build.

Added missing LibLZMA dependency when using vcpkg

d5b4be8

Fix for IsGoogleLoggingInitialized when glog is a DLL

bed6b73

asemmo added a commit to hpicgs/caffe2 that referenced this pull request Sep 28, 2017

fixes from facebookarchive#1042

ca51963

to compile with MSVC

mattm401 mentioned this pull request Oct 13, 2017

build GPU failed on windows with visual studio 2015 #1275

Closed

AurusHuang mentioned this pull request Oct 16, 2017

Error compiling caffe2_gpu under Visual Studio 2017 #1322

Closed

mattm401 mentioned this pull request Oct 18, 2017

Windows/Python Fails to load DLLs when compiled with OpenCV 3.3.0 #1349

Open

AurusHuang mentioned this pull request Nov 1, 2017

[Windows/VS]Building issues in Windows 10 related to linking #1415

Closed

aralph reviewed Nov 7, 2017

View reviewed changes

orionr closed this Sep 27, 2018

[Windows] Bug fixes for MSVC #1042

[Windows] Bug fixes for MSVC #1042

Conversation

willyd commented Aug 7, 2017

facebook-github-bot commented Aug 8, 2017

Yangqing commented Aug 8, 2017

facebook-github-bot commented Aug 8, 2017

facebook-github-bot commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yangqing left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 9, 2017

facebook-github-bot commented Aug 9, 2017

willyd commented Aug 9, 2017

Yangqing commented Aug 9, 2017

facebook-github-bot commented Aug 9, 2017

facebook-github-bot commented Aug 10, 2017

Yangqing commented Aug 10, 2017

facebook-github-bot commented Aug 10, 2017

facebook-github-bot commented Aug 10, 2017

willyd commented Aug 10, 2017

facebook-github-bot commented Aug 11, 2017

harouwu commented Aug 17, 2017

willyd commented Aug 18, 2017

Callum17 commented Aug 28, 2017 • edited

facebook-github-bot commented Aug 31, 2017

willyd commented Aug 31, 2017

pengfei666 commented Sep 4, 2017

facebook-github-bot commented Sep 12, 2017

willyd commented Sep 12, 2017

willyd commented Sep 20, 2017

hughsando commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyoic commented Dec 31, 2017

orionr commented Sep 27, 2018

Callum17 commented Aug 28, 2017 •

edited