Skip to content

[MKL-DNN] Fix to crash of Transformer when mkldnn is to be used#16233

Merged
luotao1 merged 4 commits intoPaddlePaddle:developfrom
jczaja:prv-tensor-copy-mkldnn
Mar 19, 2019
Merged

[MKL-DNN] Fix to crash of Transformer when mkldnn is to be used#16233
luotao1 merged 4 commits intoPaddlePaddle:developfrom
jczaja:prv-tensor-copy-mkldnn

Conversation

@jczaja
Copy link
Contributor

@jczaja jczaja commented Mar 15, 2019

This PR is fixing transformer analyzer test when MKL-DNN is to be used. Root cause of crash was that TensorCopy was not setting MKLDNN primitive descriptor when layout was to be kMKLDNN

Desc: TensorCopy was not setting MKLDNN primitive descriptor when layout was to be kMKLDNN

test=develop
@jczaja jczaja added NMT Intel inference Inference development labels Mar 15, 2019
@luotao1
Copy link
Contributor

luotao1 commented Mar 16, 2019

Please add TEST(Analyzer_transformer, compare_mkldnn) and TEST(Analyzer_transformer, profile_mkldnn) like analyer_resnet50_tester.cc to verity this PR. And make test_analyzer_transformer runs in SERIAL in CMAKELISTS.txt.

@jczaja jczaja requested a review from luotao1 March 18, 2019 12:56
@jczaja
Copy link
Contributor Author

jczaja commented Mar 18, 2019

@luotao1 I have updated changes with your suggestions.

if (use_mkldnn) {
cfg.EnableMKLDNN();
std::unordered_set<std::string> op_list = {"transpose2"};
cfg.SetMKLDNNOp(op_list);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we specify op_list for transformer ut? Could we remove line192-193?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luotao1 Good point. Manual selection of mkl-dnn ops for transformer was removed.

Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks very much!

@luotao1 luotao1 merged commit 13816dd into PaddlePaddle:develop Mar 19, 2019
jczaja added a commit to jczaja/Paddle that referenced this pull request Mar 27, 2019
…ed (PaddlePaddle#16233)"

This reverts commit 13816dd.
Apart from enabling transformer for MKL-DNN
jczaja added a commit to jczaja/Paddle that referenced this pull request Mar 28, 2019
…ed (PaddlePaddle#16233)"

This reverts commit 13816dd.
Apart from enabling transformer for MKL-DNN
tensor-tang pushed a commit that referenced this pull request Mar 28, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inference Inference development Intel NMT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants