MKLDNN Relu Tanh Sqrt Abs activations added

[kbinias]

• 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]

All committers have signed the CLA.

[luotao1]

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);

[kbinias]

Done

[luotao1]

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

[kbinias]

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

[luotao1]

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"

[kbinias]

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().

[luotao1]

I wonder why not inherit directly from TestRelu?

class TestMKLDNNRelu(TestRelu)

[kbinias]

Done.

[luotao1]

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.

[kbinias]

Done. I removed all my changes.

[luotao1]

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

[kbinias]

Done.

[luotao1]

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

[kbinias]

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]

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

[kbinias]

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]

@luotao1 May I ask you to continue your review.

[luotao1]

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

[luotao1]

I wonder why not inherit directly from TestRelu?

class TestMKLDNNRelu(TestRelu)

[tensor-tang]

LGTM, only one question below.

[tensor-tang]

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

[luotao1]

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