[MKL-DNN] MKL-DNN specific Tensor modification #15429

jczaja · 2019-01-18T18:36:51Z

This is first of series of PRs on tensor modifications for MKL-DNN operations that are replacing mkl-dnn format storing with MKL-DNN primitive descriptor storing.

Idea is that currently MKL-DNN format is stored in Tensor. format is not good enough for Transpose MKL-DNN as format is ambigious in terms of idenditifcations of actual mkldnn-layout. More over it does require recreation of primitive descriptions for given tensor.

Adventages:

Design should be clearer
Redundant recreation of user primitive desc should be removed

In this PR:

Tensor was modified to store MKLDNN primitive desc
Transpose op was adjusted to work with new functions
ConvGrad MKL-DNN was barely adjusted as well as Gaussian Random mkl-dnn.
All other mkl-dnn ops were not modified . They will be adjusted in next PRs (convolution will come next)
No changes to non MKL-DNN PaddlePaddle (core PaddlePaddle concepts are not modified)

jczaja · 2019-01-22T09:04:24Z

@panyx0718 To have CI passed I need your approve as PR is modifying tensor.h file. Coule you please take a look and comment/approve ?

luotao1 · 2019-01-22T12:39:34Z

Transpose op was adjusted to work with new functions

Do you have some benchmark of transpose op before and after this PR?

jczaja · 2019-01-22T13:46:26Z

@luotao1 This PR does not improve performance. It is on making Transpose MKL-DNN working
in every scenario & make foundation for cleaner design. Performance results were made using resnet50 to make sure performance did not go down. Actually with this PR it is a bit slower (x < 0.5%), but will be improved with next PR. I would like to make extensive performance tests (when I have other ops modified)

jczaja · 2019-01-22T16:14:32Z

@luotao1 I updated PR with some changes to convolution , so performance on resnet-50 (SKX 8180, B1 & B128) is restored to original level.

jczaja · 2019-01-24T08:15:02Z

@luotao1 @panyx0718 Do you need any more information to have this PR reviewed?

tensor-tang · 2019-01-24T09:28:22Z

paddle/fluid/framework/tensor.h

   */
-
-  mkldnn::memory::format format_ = mkldnn::memory::format::format_undef;
+  mutable mkldnn::memory::primitive_desc mem_pd_;
 #endif


I think there is too much MKLDNN things in base Tensor.

MKLDNNTensor is better to read for adding library.

Need @panyx0718 approve this I think.

@tensor-tang How do think about #11040 (comment)

That 's just how this current version comes from, but not the reason we cloud continue adding more.

primitive_desc contains more info like dims, format and data_type which is duplicated with many conceptions in tensor itself.

tensor-tang · 2019-01-24T10:59:55Z

paddle/fluid/framework/tensor.h


-  inline void set_format(const mkldnn::memory::format format) {
-    format_ = format;
+  void CreateMemoryPrimDescFromDims(


please unify the name style. like setMKLDNNMemPrimDesc, CreateMemoryPrimDescFromDims

MemoryPrimDesc specify the data_type, but tensor do not aware this only if get data.

It will be done

Yes. This is correct. data() / mutable_data() , returns pointer to allocation. Op has to ask separately for layout and/or Memory Primitive descriptor. Perhaps it would be good to extend function data()/ mutable_data() so they can be aware of layouts and return converted data. Definitely having data() function that can return pointer to data in Paddle Layout , would help with implementing unit test so they can operate on block layout , and then we get result in paddle layout (NCHW or other) for comparison with reference. As paddlepaddle supports many devices (CPU,GPU,MKL-DNN...) I would like someone more familiar with other devices to trigger change in interface.

panyx0718 · 2019-01-24T12:40:54Z

@sneaxiy Can you help take a look and see if there is a cleaner way?

paddle/fluid/framework/tensor.h

jczaja · 2019-01-30T14:19:33Z

@tensor-tang ,

names of functions are unified.
I tried to create MKLDNNTensor as suggessted. Eg. Inheritance chain of LODTensor -> Tensor, now becomes LODTensor -> MKLDNNtensor -> Tensor . This approach require quite alot of changes
In data_transform.cc , data_layout_layout_transform.cc and in operator.cc (possibly more..) As Tensor is quite important entity tigtly integrated with ops and data loading. So it does require alot of changes eg. ifdef PADDLE_WITH_MKLDNN code.
Other approach I tried is to Rename Tensor to TensorBase and then for non-MKLDNN , alias TensorBase as Tensor , and for mkl-dnn builds MKLDNNTensor inherits from TensorBase and MKLDNNTensor is renamed to Tensor. This approach is also quite complex , aldo debugging it later may be complex.

So what are your suggestions on how to create MKLDNNTensor so it match current design ?

panyx0718 · 2019-01-31T08:30:25Z

paddle/fluid/framework/tensor.h


-  inline void set_format(const mkldnn::memory::format format) {
-    format_ = format;
+  void create_prim_desc_from_dims(


this doesn't need to be a method of tensor? Can this be independent function and create prim_desc elsewhere and then call set_prim_desc_?

@panyx0718 I moved body of create_prim_desc_from_dims into utils file eg. mkldnn_utils.h and left thin wrapper that only calls moved out function. Is this good enough or you suggest to remove entirely create_prim_desc_from_dims() ?

tensor-tang · 2019-01-31T09:38:00Z

LODTensor -> MKLDNNtensor -> Tensor

How about MKLDNNtensor -> LODTensor -> Tensor

test=develop - TransposeMKLDNNHandler::AcquireSrcMemory was reimplemented - Added nchw and nc formats setting for sake of compatiblity Fixed unit tests - Worakaround to problem with 5D data in conv - Added 3D and 1D MKL-DNN formats for name handles for tensor test=develop - Fix to UTs test=develop - Conv fp32 op was updated Cosmetic fixes test=develop - tensor mkldnn cosmetics test=develop - Moved most of mkl-dnn specific code from Tensor to mkl-dnn utils

test=develop

jczaja · 2019-01-31T15:31:36Z

@tensor-tang I haven't tried to implement MKLDNNTensor -> LoDTensor -> Tensor, as by looking into ops that are clearly checking if Tensor is LoDTensor etc. I suspected lots of changes . So I updated PR with original concept (no MKLDNNTensor) , but I moved most of MKLDNN functionality away to mkldnn_utils.h . Eg. Tensor contains thin wrapper functions to MKLDNN specific functions.

paddle/fluid/framework/tensor.h

panyx0718 · 2019-02-01T01:44:16Z

paddle/fluid/framework/tensor.h


-  inline void set_format(const mkldnn::memory::format format) {
-    format_ = format;
+  inline mkldnn::memory::primitive_desc get_prim_desc() const {


this method and the ones above should be called mkldnn_prim_desc to make its usage more obvious.

@panyx0718 Ok, methods renamed as suggested.

test=develop

jczaja · 2019-02-01T15:04:02Z

@panyx0718 , @tensor-tang , @luotao1 I updated review following your suggestions. Please review

panyx0718

@tensor-tang @luotao1 any comments? seems ok to me.

paddle/fluid/platform/mkldnn_utils.h

paddle/fluid/operators/mkldnn/transpose_mkldnn_op.cc

paddle/fluid/platform/mkldnn_utils.h

test=develop

jczaja · 2019-02-22T08:55:54Z

@panyx0718 , @luotao1 , @tensor-tang I adjusted changes to your suggestions. Please take a look. CI cannot pass without @panyx0718 approval.

…15429)" test=develop This reverts commit dec9cf5.

* Revert "[MKL-DNN] Fix to crash of Transformer when mkldnn is to be used (#16233)" This reverts commit 13816dd. Apart from enabling transformer for MKL-DNN * Revert "- MKL-DNN pooling updated to set_prim_desc" This reverts commit c63f6b2. Conflicts: paddle/fluid/operators/mkldnn/concat_mkldnn_op.cc * Revert "[MKL-DNN] MKL-DNN specific Tensor modification (#15429)" test=develop This reverts commit dec9cf5. * - concat compilation fix - lint test=develop - Lint fixes test=develop - Lint fixes test=develop - Fix Transpose MKLDNN op test=develop

luotao1 added the Intel label Jan 21, 2019

jczaja force-pushed the prv-tensor-mkldnn branch from 9f0da40 to 5500a75 Compare January 21, 2019 10:25

jczaja requested review from panyx0718, luotao1 and tpatejko January 21, 2019 10:42

luotao1 requested a review from tensor-tang January 22, 2019 12:38

jczaja force-pushed the prv-tensor-mkldnn branch from c7b6c76 to 043d94f Compare January 22, 2019 16:15

tensor-tang reviewed Jan 24, 2019

View reviewed changes

panyx0718 requested a review from sneaxiy January 24, 2019 12:39

panyx0718 reviewed Jan 25, 2019

View reviewed changes

paddle/fluid/framework/tensor.h Show resolved Hide resolved

paddle/fluid/framework/tensor.h Show resolved Hide resolved

paddle/fluid/framework/tensor.h Show resolved Hide resolved

panyx0718 reviewed Jan 31, 2019

View reviewed changes

jczaja force-pushed the prv-tensor-mkldnn branch from 04a5d2f to f3e47bb Compare January 31, 2019 12:14

jczaja added 2 commits January 31, 2019 13:41

- Lint fixes

7e6636b

test=develop

- setting prim dec in Tensor , sets also layout to kMKLDNN

efb8396

test=develop

panyx0718 reviewed Feb 1, 2019

View reviewed changes

- Moved creation of prim desc totally out of Tensor

7b53e55

test=develop

panyx0718 reviewed Feb 18, 2019

View reviewed changes

paddle/fluid/platform/mkldnn_utils.h Outdated Show resolved Hide resolved

luotao1 reviewed Feb 18, 2019

View reviewed changes

paddle/fluid/platform/mkldnn_utils.h Show resolved Hide resolved

tensor-tang reviewed Feb 19, 2019

View reviewed changes

- Cosmetic fixes adter review

8e8c934

test=develop

jczaja force-pushed the prv-tensor-mkldnn branch from 269e605 to 8e8c934 Compare February 21, 2019 13:51

panyx0718 approved these changes Feb 25, 2019

View reviewed changes

luotao1 merged commit dec9cf5 into PaddlePaddle:develop Feb 25, 2019

jczaja mentioned this pull request Feb 25, 2019

[MKL-DNN] Adjusting ops to Tensor modifications #15917

Merged

jczaja added a commit to jczaja/Paddle that referenced this pull request Mar 27, 2019

Revert "[MKL-DNN] MKL-DNN specific Tensor modification (PaddlePaddle#…

6ee7fe1

…15429)" test=develop This reverts commit dec9cf5.

jczaja added a commit to jczaja/Paddle that referenced this pull request Mar 28, 2019

Revert "[MKL-DNN] MKL-DNN specific Tensor modification (PaddlePaddle#…

cdaa95f

…15429)" test=develop This reverts commit dec9cf5.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MKL-DNN] MKL-DNN specific Tensor modification #15429

[MKL-DNN] MKL-DNN specific Tensor modification #15429

jczaja commented Jan 18, 2019 •

edited

Loading

jczaja commented Jan 22, 2019

luotao1 commented Jan 22, 2019

jczaja commented Jan 22, 2019

jczaja commented Jan 22, 2019

jczaja commented Jan 24, 2019

tensor-tang Jan 24, 2019 •

edited

Loading

luotao1 Jan 24, 2019

tensor-tang Jan 24, 2019 •

edited

Loading

tensor-tang Jan 24, 2019 •

edited

Loading

jczaja Jan 25, 2019

panyx0718 commented Jan 24, 2019

jczaja commented Jan 30, 2019

panyx0718 Jan 31, 2019 •

edited

Loading

jczaja Jan 31, 2019

tensor-tang commented Jan 31, 2019

jczaja commented Jan 31, 2019 •

edited

Loading

panyx0718 Feb 1, 2019

jczaja Feb 1, 2019

jczaja commented Feb 1, 2019

panyx0718 left a comment

jczaja commented Feb 22, 2019

[MKL-DNN] MKL-DNN specific Tensor modification #15429

[MKL-DNN] MKL-DNN specific Tensor modification #15429

Conversation

jczaja commented Jan 18, 2019 • edited Loading

jczaja commented Jan 22, 2019

luotao1 commented Jan 22, 2019

jczaja commented Jan 22, 2019

jczaja commented Jan 22, 2019

jczaja commented Jan 24, 2019

tensor-tang Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

luotao1 Jan 24, 2019

Choose a reason for hiding this comment

tensor-tang Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

tensor-tang Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

jczaja Jan 25, 2019

Choose a reason for hiding this comment

panyx0718 commented Jan 24, 2019

jczaja commented Jan 30, 2019

panyx0718 Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

jczaja Jan 31, 2019

Choose a reason for hiding this comment

tensor-tang commented Jan 31, 2019

jczaja commented Jan 31, 2019 • edited Loading

panyx0718 Feb 1, 2019

Choose a reason for hiding this comment

jczaja Feb 1, 2019

Choose a reason for hiding this comment

jczaja commented Feb 1, 2019

panyx0718 left a comment

Choose a reason for hiding this comment

jczaja commented Feb 22, 2019

jczaja commented Jan 18, 2019 •

edited

Loading

tensor-tang Jan 24, 2019 •

edited

Loading

tensor-tang Jan 24, 2019 •

edited

Loading

tensor-tang Jan 24, 2019 •

edited

Loading

panyx0718 Jan 31, 2019 •

edited

Loading

jczaja commented Jan 31, 2019 •

edited

Loading