MKLDNN Relu Tanh Sqrt Abs activations added #9081

kbinias · 2018-03-14T13:56:18Z

Implements Relu, Tanh, Sqrt, Abs activations
Passes unit tests for Relu, Tanh, Sqrt, Abs

Beside unit tests, we validated these kernels by running training and interference on MNIST dataset and comparing results with IntelCaffe library.

CLAassistant · 2018-03-14T13:56:24Z

All committers have signed the CLA.

luotao1 · 2018-03-16T11:34:07Z

paddle/fluid/operators/activation_mkldnn_op.cc

+  __macro(relu, ReluMkldnnFunctor, ReluMkldnnGradFunctor)         \
+      __macro(tanh, TanhMkldnnFunctor, TanhMkldnnGradFunctor)     \
+          __macro(sqrt, SqrtMkldnnFunctor, SqrtMkldnnGradFunctor) \
+              __macro(abs, AbsMkldnnFunctor, AbsMkldnnGradFunctor);


Could line 187-190 be left-justified?

#define FOR_EACH_MKLDNN_KERNEL_FUNCTOR(__macro) \ __macro(relu, ReluMkldnnFunctor, ReluMkldnnGradFunctor) \ __macro(tanh, TanhMkldnnFunctor, TanhMkldnnGradFunctor) \ __macro(sqrt, SqrtMkldnnFunctor, SqrtMkldnnGradFunctor) \ __macro(abs, AbsMkldnnFunctor, AbsMkldnnGradFunctor);

luotao1 · 2018-03-16T11:36:50Z

paddle/fluid/operators/activation_op.cc

+    AddAttr<bool>("use_mkldnn",
+                  "(bool, default false) Only used in mkldnn kernel")
+        .SetDefault(false);
+    AddAttr<std::string>(


Why activation_op has the data_format? They should not consider NCHW or NHCW.

Done. Yes, it doesn't matter for MKLDNN eltwise.

luotao1 · 2018-03-16T11:48:03Z

python/paddle/fluid/tests/unittests/test_activation_op.py

@@ -484,5 +484,72 @@ def test_check_grad(self):
        self.check_grad(['X'], 'Out', max_relative_error=0.008)


+#--------------------test MKLDNN--------------------
+class TestMKLDNNRelu(OpTest):


Could you re-use the above unit-tests, and create MKDNN related unit-tests like

Paddle/python/paddle/fluid/tests/unittests/test_conv2d_op.py

Lines 377 to 380 in e967d19

class TestMKLDNN(TestConv2dOp):

def init_op_type(self):

self.use_mkldnn = True

self.op_type = "conv2d"

Tests in test_activation_op.py inherit directly from OpTest.
I had to send to numpy uniform different size (e.g. [2, 4, 3, 5]) so I needed to use setUp().

I wonder why not inherit directly from TestRelu?

class TestMKLDNNRelu(TestRelu)

luotao1 · 2018-03-16T11:52:05Z

python/paddle/fluid/tests/unittests/op_test.py

@@ -215,7 +215,8 @@ def setUpClass(cls):
        '''Fix random seeds to remove randomness from tests'''
        cls._np_rand_state = np.random.get_state()
        cls._py_rand_state = random.getstate()
-
+        cls.use_mkldnn = False


What's the reason for updating the op_test.py? Adding MKLDNN unit-tests (both conv2d #8451 and pool ##8879) don't need to modify the op_test.py.

Done. I removed all my changes.

luotao1 · 2018-03-16T12:01:43Z

paddle/fluid/operators/activation_op.h

@@ -49,6 +76,27 @@ class ActivationKernel
  }
 };

+template <typename Functor>


Could MKLDNNActivationKernel and MKLDNNActivationGradKernel move to a new file named mkldnn_activation_op.h?

luotao1 · 2018-03-16T12:13:54Z

paddle/fluid/platform/mkldnn_helper.h

@@ -42,6 +42,7 @@ inline mkldnn::memory::desc MKLDNNMemDesc(const std::vector<int>& dims,
 }

 inline bool CanMKLDNNBeUsed(const framework::ExecutionContext& ctx) {
+  if (!ctx.HasAttr("use_mkldnn")) return false;


What's the purpose of line 45? Why don't previous conv2d #8451 and pool #8879 PR need this line?

Done. I removed all my changes. The problem was with not MKLDNN activation operators. The file activation_op.cc contains mix of operators (all for CPU and CUDA and 4 for MKLDNN). They don't have by default required MKLDNN attribute (e.g. AddAttr("use_mkldnn"), "..."). I added new ActivationWithMKLDNNOp and ActivationWithMKLDNNOpGrad classes to support this situation. An alternative solution may be adding use_mkldnn attribute to all activations operators but it seems to be ugly.

luotao1 · 2018-03-16T12:17:25Z

paddle/fluid/framework/operator.h

@@ -195,6 +199,10 @@ class ExecutionContext {
    return op_.Attr<T>(name);
  }

+  inline bool HasAttr(const std::string& name) const {
+    return op_.HasAttr(name);
+  }


What's the purpose of adding HasAttr method? Why don't previous conv2d #8451 and pool #8879 PR need this method?

Done. I removed all my changes. The idea was that it would be nice to have a function that would check existence of attribute in hashtable.

kbinias · 2018-03-22T10:10:21Z

@luotao1 May I ask you to continue your review.

luotao1

@tensor-tang Can you help review activation_mkldnn_op.cc?

luotao1 · 2018-03-22T12:04:10Z

python/paddle/fluid/tests/unittests/test_activation_op.py

@@ -484,5 +484,72 @@ def test_check_grad(self):
        self.check_grad(['X'], 'Out', max_relative_error=0.008)


+#--------------------test MKLDNN--------------------
+class TestMKLDNNRelu(OpTest):


I wonder why not inherit directly from TestRelu?

class TestMKLDNNRelu(TestRelu)

tensor-tang

LGTM, only one question below.

tensor-tang · 2018-03-15T03:07:31Z

paddle/fluid/operators/CMakeLists.txt

@@ -127,7 +127,12 @@ function(op_library TARGET)

    # pybind USE_OP_DEVICE_KERNEL for MKLDNN
    if (WITH_MKLDNN AND ${mkldnn_cc_srcs_len} GREATER 0)
+      # Append first implemented MKLDNN activation operator
+      if (${MKLDNN_FILE} STREQUAL "activation_mkldnn_op")
+        file(APPEND ${pybind_file} "USE_OP_DEVICE_KERNEL(relu, MKLDNN);\n")


The condition is activation_mkldnn_op, so why named as "relu" which is just one of activation.

The reason is that it's enough to just adding one operator to pybind in a *_op.cc file:

Paddle/paddle/fluid/operators/CMakeLists.txt

Lines 92 to 102 in 7c1a0b7

# Note that it's enough to just adding one operator to pybind in a *_op.cc file.

# And for detail pybind information, please see generated paddle/pybind/pybind.h.

file(READ ${TARGET}.cc TARGET_CONTENT)

string(REGEX MATCH "REGISTER_OP\\(.*REGISTER_OP\\(" multi_register "${TARGET_CONTENT}")

string(REGEX MATCH "REGISTER_OP\\([a-z0-9_]*," one_register "${multi_register}")

if (one_register STREQUAL "")

string(REPLACE "_op" "" TARGET "${TARGET}")

else ()

string(REPLACE "REGISTER_OP(" "" TARGET "${one_register}")

string(REPLACE "," "" TARGET "${TARGET}")

endif()

luotao1

LGTM

kbinias added the Intel label Mar 14, 2018

kbinias requested a review from tensor-tang March 14, 2018 13:56

kbinias force-pushed the kbinias/mkldnn-activations branch from b82071e to acfaa3c Compare March 14, 2018 15:11

luotao1 reviewed Mar 16, 2018

View reviewed changes

kbinias removed the request for review from tensor-tang March 20, 2018 09:12

kbinias force-pushed the kbinias/mkldnn-activations branch 2 times, most recently from 8be8def to aad5df7 Compare March 21, 2018 22:11

luotao1 reviewed Mar 22, 2018

View reviewed changes

kbinias force-pushed the kbinias/mkldnn-activations branch from b0f9f4e to 6810ca9 Compare March 22, 2018 19:21

kbinias added 4 commits March 23, 2018 14:37

MKLDNN Relu Tanh Sqrt Abs activations added

4466f0b

Correcting for PR comments

a64b312

Fixed tests

d8bd436

Inheritance added for MKLDNN tests

6461e80

kbinias force-pushed the kbinias/mkldnn-activations branch from 6810ca9 to 6461e80 Compare March 23, 2018 13:38

kbinias mentioned this pull request Mar 24, 2018

Can't alloc enough memory when run test. #9347

Closed

tensor-tang reviewed Mar 25, 2018

View reviewed changes

luotao1 approved these changes Mar 26, 2018

View reviewed changes

luotao1 merged commit cb3bbbd into PaddlePaddle:develop Mar 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MKLDNN Relu Tanh Sqrt Abs activations added #9081

MKLDNN Relu Tanh Sqrt Abs activations added #9081

kbinias commented Mar 14, 2018

CLAassistant commented Mar 14, 2018 •

edited

Loading

luotao1 Mar 16, 2018

kbinias Mar 20, 2018

luotao1 Mar 16, 2018

kbinias Mar 20, 2018

luotao1 Mar 16, 2018

kbinias Mar 20, 2018 •

edited

Loading

luotao1 Mar 22, 2018

kbinias Mar 22, 2018

luotao1 Mar 16, 2018

kbinias Mar 20, 2018

luotao1 Mar 16, 2018

kbinias Mar 20, 2018

luotao1 Mar 16, 2018

kbinias Mar 20, 2018 •

edited

Loading

luotao1 Mar 16, 2018

kbinias Mar 20, 2018

kbinias commented Mar 22, 2018

luotao1 left a comment

luotao1 Mar 22, 2018

tensor-tang left a comment

tensor-tang Mar 15, 2018

luotao1 Mar 26, 2018

luotao1 left a comment

	class TestMKLDNN(TestConv2dOp):
	def init_op_type(self):
	self.use_mkldnn = True
	self.op_type = "conv2d"

	# Note that it's enough to just adding one operator to pybind in a *_op.cc file.
	# And for detail pybind information, please see generated paddle/pybind/pybind.h.
	file(READ ${TARGET}.cc TARGET_CONTENT)
	string(REGEX MATCH "REGISTER_OP\\(.*REGISTER_OP\\(" multi_register "${TARGET_CONTENT}")
	string(REGEX MATCH "REGISTER_OP\\([a-z0-9_]*," one_register "${multi_register}")
	if (one_register STREQUAL "")
	string(REPLACE "_op" "" TARGET "${TARGET}")
	else ()
	string(REPLACE "REGISTER_OP(" "" TARGET "${one_register}")
	string(REPLACE "," "" TARGET "${TARGET}")
	endif()

MKLDNN Relu Tanh Sqrt Abs activations added #9081

MKLDNN Relu Tanh Sqrt Abs activations added #9081

Conversation

kbinias commented Mar 14, 2018

CLAassistant commented Mar 14, 2018 • edited Loading

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

kbinias Mar 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbinias Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbinias commented Mar 22, 2018

luotao1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tensor-tang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luotao1 left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 14, 2018 •

edited

Loading

kbinias Mar 20, 2018 •

edited

Loading

kbinias Mar 20, 2018 •

edited

Loading