-
Notifications
You must be signed in to change notification settings - Fork 18.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MKL/non-MKL merge #97
Conversation
- examples, test and pycaffe compile without problem (matcaffe not tested) - tests show some errors (on cpu gradient tests), to be investigated - random generators need to be double checked - mkl commented code needs to be removed
previously filled in all NaNs for me, making many tests fail)
Thanks! This is a major step in reconciling MKL and non-MKL Caffe. I'll review and merge soon, with a rebase for grooming to keep the history clear. |
The new PR is faster than MKL in #85. |
@@ -7,6 +7,8 @@ CUDA_ARCH := -gencode arch=compute_20,code=sm_20 \ | |||
-gencode arch=compute_30,code=sm_30 \ | |||
-gencode arch=compute_35,code=sm_35 | |||
|
|||
# If not using MKL, comment out the following line. | |||
# USE_MKL=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check if the
ifdef USE_MKL
in Makefile will be false if USE_MKL=0 (I suspect not)? There might be a
hidden bug here by making the if statement ambiguous. Essentially
we should be checking the value of USE_MKL instead of checking whether it
is defined.
I am sure you are more familiar with Makefile jargons so please kindly fix
it :)
Yangqing
On Fri, Feb 14, 2014 at 1:19 PM, Evan Shelhamer notifications@github.comwrote:
In Makefile.config.example:
@@ -7,6 +7,8 @@ CUDA_ARCH := -gencode arch=compute_20,code=sm_20
-gencode arch=compute_30,code=sm_30
-gencode arch=compute_35,code=sm_35+# If not using MKL, comment out the following line.
+# USE_MKL=1TODO this should be =0. I'll fix during merge (real soon now).
Reply to this email directly or view it on GitHubhttps://github.com//pull/97/files#r9764360
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shouldn't work on Caffe before I have my coffee :) I'll make sure it's right when I merge.
Confirmed builds and tests for MKL and non-MKL on linux. On OS X 10.9, this breaks with boost errors when building the cuda .cu:
This is for both the MLK and non-MKL builds. |
Excellent integration @rowlanddepp ! |
Isn't @rowlanddepp == @Yangqing? |
Sync with latest BVLC
(Note: the reason you are seeing a lot of files being changed is because I also did a merge from the master branch. The changes that are not already in the master branch could be seen at:
rowlanddepp@9bba820
)
A major rewrite of all the MKL vsl interfaces. This commit allows:
(1) recoverting the code back to a uniform VSL interface. With MKL present, VSL functions are called. With no MKL, functions defined in include/caffe/util/mkl_alternate.hpp kicks in and work as a simple backend. As most computation time is spent on gemm, the simple implementation does not lead to noticeable computation overhead.
(2) As a result, the current code only relies on basic blas, and eigen is not required.
(3) A simple flag USE_MKL to control whether to use MKL or not.
(4) If one wants to implement better VSL functions, only mkl_alternate.hpp needs to be changed, allowing future codes to be more modularized.
(5) Reverted back to using atlas and cblas, since the default openblas on ubuntu is not stable yet. One can simply change the link library to use openblas if desired.
Hopefully this will help us merge boost-eigen and the master thread more easily, and help MKL and non MKL to coexist in the same branch with little maintenance overhead.