-
Notifications
You must be signed in to change notification settings - Fork 6.8k
support mkl log when dtype is fp32 or fp64 #13150
Conversation
@szha @eric-haibin-lin could you help take a review? |
@mxnet-label-bot [pr-awaiting-review] |
fccc7eb
to
5c5c8f7
Compare
@nswamy @sandeep-krishnamurthy @szha could you please help with reviewing the PR? |
@azai91 @mseth10 @samskalicky ping for review |
@mxnet-label-bot add [MKL] |
@TaoLv please help take a review, thanks. |
|
||
#if MSHADOW_USE_MKL == 1 | ||
#define MKLLOG(fname, DType) \ | ||
static void MKLLog(size_t size, const DType* pIn, DType* pOut) { \ |
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.
Macro has the same name with function. Using macro here seems a little bit overkill for me. How about:
#if MSHADOW_USE_MKL == 1
static inline void MKL_Log(int size, const float* pIn, float* pOut) {
vsLn(size, pIn, pOut);
}
static inline void MKL_Log(int size, const double* pIn, double* pOut) {
vdLn(size, pIn, pOut);
}
#endif
Also, please note that vsLn/vdLn
take MKL_INT
as the first input. size_t
might overflow.
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.
Thanks, it's done
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.
Thanks for your contributions.
Are there existing test cases to test log with mkl? Or, do we need more tests to be added?
@@ -940,7 +940,7 @@ The storage type of ``exp`` output is always dense | |||
.set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseOut{"_mul"}); | |||
|
|||
// log | |||
MXNET_OPERATOR_REGISTER_UNARY_WITH_SPARSE_DR(log, cpu, mshadow_op::log) | |||
MXNET_OPERATOR_REGISTER_UNARY(log) |
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.
Sorry, I do not understand this change. Can you please explain?
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.
the defination of MXNET_OPERATOR_REGISTER_UNARY_WITH_SPARSE_DR
in elemwise_unary_op.h
is here:
#define MXNET_OPERATOR_REGISTER_UNARY_WITH_SPARSE_DR(__name$, __xpu$, __kernel$) \
MXNET_OPERATOR_REGISTER_UNARY(__name$) \
.set_attr<FCompute>("FCompute<" #__xpu$ ">", UnaryOp::Compute<__xpu$, __kernel$>)
it will set FCompute attribute to UnaryOp::Compute
automatically. To call Our LogCompute
, we replace it with below codes.
MXNET_OPERATOR_REGISTER_UNARY(log)
.set_attr<FCompute>("FCompute<cpu>", UnaryOp::LogCompute<cpu, mshadow_op::log>)
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.
lgtm pending other comments to address
@@ -348,6 +351,40 @@ class UnaryOp : public OpBase { | |||
LogUnimplementedOp(attrs, ctx, inputs, req, outputs); | |||
} | |||
} | |||
|
|||
#if MSHADOW_USE_MKL == 1 | |||
static inline void MKL_Log(int size, const float* pIn, float* pOut) { |
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.
int: rename to MKLLog
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.
@eric-haibin-lin Do we have any recommended convention for function/class name definition in MXNet? Just not feel very comfortable with 4 consecutive capital letters. :(
vsLn(size, pIn, pOut); | ||
} | ||
|
||
static inline void MKL_Log(int size, const double* pIn, double* pOut) { |
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.
MKL_Log
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.
A question (outside the scope of this PR) to the intel team. Is there any plan to add MKL specific tests to the CI ? Currently we are completely blind to it and we have also experienced issues reported but cannot be caught in PRs.
@anirudh2290 good question. |
@pengzhao-intel Thanks for the info. Sounds good ! |
@XiaotaoChen since #13607 is merged, please rebase code and retrigger CI. Make sure that unit test for log operator can correctly run into MKL BLAS. |
const size_t MKL_INT_MAX = (sizeof(MKL_INT) == sizeof(int)) ? INT_MAX : LLONG_MAX; | ||
size_t input_size = inputs[0].Size(); | ||
if (req[0] == kWriteTo && (type_flag == mshadow::kFloat32 | ||
|| type_flag == mshadow::kFloat64) && input_size <= MKL_INT_MAX) { |
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.
fix indent.
|| type_flag == mshadow::kFloat64) && input_size <= MKL_INT_MAX) { | ||
MSHADOW_SGL_DBL_TYPE_SWITCH(type_flag, DType, { | ||
MKLLog(input_size, inputs[0].dptr<DType>(), outputs[0].dptr<DType>()); | ||
}) |
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.
need ;
here.
#if MSHADOW_USE_MKL == 1 | ||
#include "mkl.h" | ||
#endif | ||
#include<climits> |
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.
need space before <
and put this include before #include "./cast_storage-inl.h"
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.
all is done
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.
LGTM. Seems CI status is not reflected on the page. Could you retrigger it by an empty commit?
Thank you for the contribution. Now merging. |
Description
kWriteTo
,mx.nd.log
will call MKL Log. otherwise, it will call the default implementation.nd.log
gains 2.67 times speedup compared with the default implementation. the total time of log operator dropped from 11.5s to 4.3s. profile info as below.@pengzhao-intel
detail data
hardware: skx-8180 single socket(28 cores)
statistics in c++ code
statistics in python front end
here is test script
sockeye profile on skx-8180 single socket
offical master
mkl log