Legacy nvcc support #1236

mlapin · 2014-10-07T21:00:10Z

Hey,

unfortunately, commit 0ba046b (merge PR #1070) breaks compilation in our legacy environment (we currently use cuda toolkit 5.0 and nvcc with gcc 4.4). The compilation errors we get are of the following type:

emmintrin.h(1312): error: identifier "__builtin_ia32_vec_ext_v8hi" is undefined

This could be related to a similar issue with intrinsics when compiling under OSX (mentioned in #1070).

The proposed fix is to avoid inclusion of the opencv header when compiling with nvcc and the host gcc is < 4.7.

I kept the original workaround with the OSX flag for now, but my guess is the whole check could be simplified as:

#if __NVCC__
namespace cv {class Mat;}
#else
#include <opencv2/core/core.hpp>
#endif

that is, the cuda code does not actually require opencv, except for the class declaration cv::Mat.

The downside of this approach is that ReadImageToCVMat is not inlined.

sguada · 2014-10-08T01:23:57Z

@mlapin Thanks for your PR. I agree that it seems that OSX could be simplified to NVCC but that needs to be tested.

Could you check if #1239 also breaks your legacy code and if the same fix can be applied?

mlapin · 2014-10-08T10:03:49Z

I have simplified the fix by removing OSX and using the __NVCC__ instead.

@sguada I have checked #1239 and yes, it also breaks our build, and yes, it can be fixed the same way:

do not include opencv2/core/core.hpp when compiling with nvcc;
do not inline functions that return cv::Mat

mlapin · 2014-10-08T12:13:38Z

On second thought, there is an even simpler solution:

include opencv in source (.cpp) files only, not in headers.

This way there's no need for conditional includes, hence no macros.

Warning: this PR needs to be tested in OSX.

Further details:
Since PR #1070 (and #1239) introduces opencv dependency in Caffe's public API (e.g. functions ReadImageToCVMat() return cv::Mat and are tested as libcaffe's API), this has to be handled somehow.

If we avoid opencv includes in headers (which breaks compilation with legacy nvcc), we need a forward declaration of cv::Mat which I currently put in common.hpp. Since cv::Mat is thus not yet a complete type, there should be no function definitions in headers using that type. Hence, inlined ReadImageToCVMat() had to go.

A potentially better approach would be to drop the opencv dependency in Caffe's public API by introducing a wrapper for cv::Mat. This way we would avoid any mention of opencv in Caffe headers.

shelhamer · 2014-10-08T16:35:18Z

A potentially better approach would be to drop the opencv dependency in Caffe's public API by introducing a wrapper for cv::Mat. This way we would avoid any mention of opencv in Caffe headers.

This was the approach taken for boost RNG when it conflicted in this same way with NVCC. It is more work to have a dummy class for nothing but legacy compatibility. Ideally we can go without it, since the RNG class was comparatively simple compared to cv::Mat, but the option is there.

shelhamer · 2014-10-08T16:46:34Z

@sguada dropping the problematic OpenCV includes from the headers seems like a reasonable fix and this works for me on OS X 10.9. Please double-check and merge.

kloudkl · 2014-10-13T14:39:30Z

OpenCV has a CUDA module in itself. How does it build successfully?

sguada · 2014-12-09T22:27:11Z

include/caffe/common.hpp

This solution doesn't work for vectorcv::Mat, any idea why?

This line simply declares "there's class Mat in namespace cv". Clearly, it won't work for other namespaces, like the vectorcv. Now, I'm not sure about the details here (what is the vectorcv?), but one way to fix it could be to add a similar line with the vectorcv namespace.

@mlapin I think @sguada meant vectorcv::Mat

@bhack thanks, I also suspected it, but then it should actually work (and it does work for me - see below).
@sguada I would need more details to help here - what exactly doesn't work?

This compiles on my machine:
in data_transformer.hpp:
void Transform(const vector<cv::Mat>& cv_img, Blob<Dtype>* transformed_blob);
in data_transformer.cpp:

template<typename Dtype> void DataTransformer<Dtype>::Transform(const vector<cv::Mat>& cv_img, Blob<Dtype>* transformed_blob) { for (int item_id = 0; item_id < cv_img.size(); ++item_id) { Transform(cv_img[item_id], transformed_blob); } }

Drop OpenCV includes from NVCC code for legacy reasons.

shelhamer · 2015-01-16T05:12:02Z

This ended up fine on OS X. While I'm not so enthused to bring an OpenCV declaration into common.hpp if it helps compatibility it's fine for now.

Nerei · 2015-01-17T21:03:03Z

@sguada

This solution doesn't work for vectorcv::Mat, any idea why?

I guess if you use vector::resize() which takes matrix by value, class definition is required.

shelhamer force-pushed the dev branch from d8eb4df to 914da95 Compare October 8, 2014 16:36

shelhamer assigned sguada Oct 8, 2014

shelhamer added the compatibility label Oct 8, 2014

shelhamer mentioned this pull request Oct 10, 2014

Opencv io support #1061

Closed

sergeyk force-pushed the dev branch from 2fb4c97 to 1718903 Compare October 17, 2014 18:44

include opencv only in .cpp

c211fd0

sguada reviewed Dec 9, 2014
View reviewed changes

shelhamer merged commit c211fd0 into BVLC:dev Jan 16, 2015

shelhamer added a commit that referenced this pull request Jan 16, 2015

Merge pull request #1236 from mlapin/legacy_nvcc_support

7695db9

Drop OpenCV includes from NVCC code for legacy reasons.

This was referenced Jan 16, 2015

Added opencv vector<Mat> to memory data layer with tests #1416

Closed

Improved CMake scripts #1667

Closed

Legacy nvcc support #1236

Legacy nvcc support #1236

Uh oh!

Conversation

mlapin commented Oct 7, 2014

Uh oh!

sguada commented Oct 8, 2014

Uh oh!

mlapin commented Oct 8, 2014

Uh oh!

mlapin commented Oct 8, 2014

Uh oh!

shelhamer commented Oct 8, 2014

Uh oh!

shelhamer commented Oct 8, 2014

Uh oh!

kloudkl commented Oct 13, 2014

Uh oh!

sguada Dec 9, 2014

Choose a reason for hiding this comment

Uh oh!

mlapin Dec 10, 2014

Choose a reason for hiding this comment

Uh oh!

bhack Dec 10, 2014

Choose a reason for hiding this comment

Uh oh!

mlapin Dec 10, 2014

Choose a reason for hiding this comment

Uh oh!

shelhamer commented Jan 16, 2015

Uh oh!

Nerei commented Jan 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants