[GitHub] asmushetzel commented on a change in pull request #8846: Batching improvements for GEMM/TRSM operators and full MKL usage docs.

[GitBox <gi...@apache.org>]

asmushetzel commented on a change in pull request #8846: Batching improvements for GEMM/TRSM operators and full MKL usage docs.
URL: https://github.com/apache/incubator-mxnet/pull/8846#discussion_r153476788
 
 

 ##########
 File path: MKL_README.md
 ##########
 @@ -1,3 +1,21 @@
+# Full MKL Installation
 
 Review comment:
   You should mention the purpose of doing so, i.e. that this will enable MKL for all operators in the linalg-namespace.
   
   What about this piece of code (I guess it is still in the config):
   # by default, disable lapack when using MKL
   # switch on when there is a full installation of MKL available (not just MKL2017/MKL_ML)
   ifeq ($(USE_BLAS), mkl)
   USE_LAPACK = 0
   endif
   
   Guess this has to be changed as well. 
   
   And unfortunately this does not work exactly as planned. With the suggested settings, a user would get MKL for blas/lapack, but same time setting USE_MKL2017=0 would internally switch off use of MKLML for a alot of NN operators.  Setting USE_MKL2017=0 was just a shortcut for our experiments with the linalg-operators. 
   
   The mechanism ideally should work such that the user just sets USE_BLAS=mkl and that is it. He can in addition set USE_MKL2017 and then also some other operators will start useing MKL's NN-functions. 
   
   
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services