[Intel MKL] Enable FP32 FusedMatMul for MKL-DNN. #31782

Zantares · 2019-08-20T02:43:15Z

Enable FP32 FusedMatMul(MatMul with BiasAdd) for MKL-DNN backend according to below rule:

No transpose attribute with first input
FusedMatMul op only has 1 post op;

Also added several cases for testing, moved common code from QuantizedMatMul to an head file.

Modified:

tensorflow/core/graph/mkl_layout_pass.cc
tensorflow/core/graph/mkl_layout_pass_test.cc
tensorflow/core/grappler/optimizers/remapper.cc
tensorflow/core/kernels/BUILD
tensorflow/core/kernels/mkl_fused_ops_test.cc
tensorflow/core/kernels/mkl_qmatmul_op.cc
tensorflow/core/ops/mkl_nn_ops.cc
tensorflow/core/util/mkl_util.h

Added:

tensorflow/core/kernels/mkl_matmul_op_fused.cc
tensorflow/core/kernels/mkl_matmul_ops_common.h

Signed-off-by: Lu Teng teng.lu@intel.com

mellanox-github · 2019-08-20T13:02:52Z

Can one of the admins verify this patch?

gbaned · 2019-08-27T10:17:11Z

@Zantares Could you please resolve the conflicts? Thanks!

Zantares · 2019-08-30T09:00:08Z

@Zantares Could you please resolve the conflicts? Thanks!

Solved, thanks!

penpornk

I'm so sorry for the delay! I have mostly minor comments.

tensorflow/core/graph/mkl_layout_pass.cc

penpornk · 2019-09-03T22:52:01Z

tensorflow/core/graph/mkl_layout_pass.cc

+
+    // Do not rewrite with transpose attribute because reorder has performance
+    // impact.
+    TF_CHECK_OK(GetNodeAttr(n->def(), "transpose_a", &trans_a));


What about transposing B? Are we assuming B is small and would be cheap to reorder?

Input B(weight) can be easily pass to MKL-DNN with different memory desc(io/oi) to represent the transpose attribute. It doesn't need reorder here.

Got it. Thank you for the explanation!

tensorflow/core/kernels/mkl_matmul_op_fused.cc

tensorflow/core/kernels/mkl_matmul_ops_common.h

tensorflow/core/ops/mkl_nn_ops.cc

Zantares · 2019-09-05T03:59:34Z

@penpornk thanks for your review, I'll spend 1~2 days to refine the code and ping you once I finish the work:)

penpornk · 2019-09-10T22:31:06Z

tensorflow/core/graph/mkl_layout_pass.cc

+
+    // Do not rewrite with transpose attribute because reorder has performance
+    // impact.
+    TF_CHECK_OK(GetNodeAttr(n->def(), "transpose_a", &trans_a));


Got it. Thank you for the explanation!

gbaned · 2019-09-11T12:33:13Z

@Zantares Could you please address Ubuntu Sanity errors? Thanks!

Zantares · 2019-09-11T14:37:53Z

@Zantares Could you please address Ubuntu Sanity errors? Thanks!

I've located the failure. It caused by comma missing in BUILD file.

Hi @penpornk , I'm sorry that I didn't ping you after refining the code, because I found I can handle the fusion of MatMul, BiasAdd and Relu via reusing code here: https://github.com/tensorflow/tensorflow/pull/31782/files#diff-4e6ad12b109cad067443f126cc8f2145R187.
It's not a really fusion and just execute Relu after MatMulWithBias in same MKL stream, however it may bring some benefit via cache locality. Do you have some suggestion if I should add this feature into this PR? I mean whether I should split it to another PR because you've already approved this PR now.

penpornk

If the changes are minor, please feel free to add them to this PR. (I actually found a typo but didn't want to request changes earlier. I might as well request it now.) Otherwise, please open a new PR. :)

penpornk · 2019-09-11T21:05:50Z

tensorflow/core/kernels/mkl_matmul_op_fused.cc

+    const int channel = weight_tf_shape.dim_size(1 - dim_pair[1]);
+
+    OP_REQUIRES(ctx, k == weight_tf_shape.dim_size(dim_pair[1]),
+                errors::InvalidArgument("Matrix size are incompatible: In[0]: ",


Suggested change

errors::InvalidArgument("Matrix size are incompatible: In[0]: ",

errors::InvalidArgument("Matrix sizes are incompatible: In[0]: ",

I've reverted the change here because there's a case checking the error verbose like this: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/eager/function_test.py#L270

penpornk · 2019-09-11T21:10:35Z

And no worries about pinging! I saw that you added a commit that addressed all the comments so I assumed the PR is ready for review. Sorry for not waiting until you say so!

Zantares · 2019-09-23T03:49:41Z

Hi @penpornk , we decide to separate Relu fusion from this PR after internal discussion. Besides, I'v tested Tensorflow UT locally and fixed some errors, please take a look at the new commit, thanks!

penpornk

Got it. Thanks again for your PR!

PiperOrigin-RevId: 270955757

Enable FP32 FusedMatMul for MKL-DNN.

edfb680

tensorflow-bot bot added the size:XL CL Change Size:Extra Large label Aug 20, 2019

googlebot added the cla: yes label Aug 20, 2019

gbaned self-assigned this Aug 20, 2019

gbaned added the comp:mkl MKL related issues label Aug 20, 2019

gbaned added this to Assigned Reviewer in PR Queue via automation Aug 20, 2019

gbaned requested a review from penpornk August 20, 2019 05:31

gbaned added the stat:awaiting response Status - Awaiting response from author label Aug 27, 2019

Merge branch 'Intel-TF/master' into tenglu/fuse_fp32_matmul

e8001f3

tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Aug 31, 2019

gbaned added the awaiting review Pull request awaiting review label Sep 3, 2019

penpornk requested changes Sep 4, 2019

View reviewed changes

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Sep 4, 2019

tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 4, 2019

Refine MKL-DNN FusedMatMul code according to review.

c1c1765

gbaned requested a review from penpornk September 10, 2019 10:38

penpornk previously approved these changes Sep 10, 2019

View reviewed changes

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 10, 2019

tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 10, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 10, 2019

Fix Ubuntu Sanity errors for MKL-DNN FusedMatMul.

ebbbee4

Zantares dismissed penpornk’s stale review via ebbbee4 September 11, 2019 15:07

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Sep 11, 2019

penpornk requested changes Sep 11, 2019

View reviewed changes

gbaned added awaiting review Pull request awaiting review and removed ready to pull PR ready for merge process labels Sep 13, 2019

penpornk added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Sep 19, 2019

Zantares added 3 commits September 23, 2019 11:26

Merge branch 'master' into tenglu/fuse_fp32_matmul

9bcad70

Fix UT failures for fp32 MklFusedMatMul.

94ae1e0

Merge branch 'master' into tenglu/fuse_fp32_matmul

bc0b234

tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Sep 23, 2019

penpornk approved these changes Sep 23, 2019

View reviewed changes

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 23, 2019

tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 23, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 23, 2019

gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Sep 24, 2019

tensorflow-copybara pushed a commit that referenced this pull request Sep 24, 2019

Merge pull request #31782 from Intel-tensorflow:tenglu/fuse_fp32_matmul

8e3f8ae

PiperOrigin-RevId: 270955757

tensorflow-copybara merged commit bc0b234 into tensorflow:master Sep 24, 2019

PR Queue automation moved this from Approved by Reviewer to Merged Sep 24, 2019

Zantares deleted the tenglu/fuse_fp32_matmul branch September 25, 2019 01:48

Zantares mentioned this pull request Oct 17, 2019

[Intel MKL] Add FP32 fusion of MatMul and Relu. #33451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Intel MKL] Enable FP32 FusedMatMul for MKL-DNN. #31782

[Intel MKL] Enable FP32 FusedMatMul for MKL-DNN. #31782

Zantares commented Aug 20, 2019

mellanox-github commented Aug 20, 2019

gbaned commented Aug 27, 2019

Zantares commented Aug 30, 2019

penpornk left a comment

penpornk Sep 3, 2019

Zantares Sep 5, 2019

penpornk Sep 10, 2019

Zantares commented Sep 5, 2019

penpornk Sep 10, 2019

gbaned commented Sep 11, 2019

Zantares commented Sep 11, 2019 •

edited

penpornk left a comment

penpornk Sep 11, 2019

Zantares Sep 23, 2019

penpornk commented Sep 11, 2019

Zantares commented Sep 23, 2019

penpornk left a comment

	errors::InvalidArgument("Matrix size are incompatible: In[0]: ",
	errors::InvalidArgument("Matrix sizes are incompatible: In[0]: ",

[Intel MKL] Enable FP32 FusedMatMul for MKL-DNN. #31782

[Intel MKL] Enable FP32 FusedMatMul for MKL-DNN. #31782

Conversation

Zantares commented Aug 20, 2019

mellanox-github commented Aug 20, 2019

gbaned commented Aug 27, 2019

Zantares commented Aug 30, 2019

penpornk left a comment

Choose a reason for hiding this comment

penpornk Sep 3, 2019

Choose a reason for hiding this comment

Zantares Sep 5, 2019

Choose a reason for hiding this comment

penpornk Sep 10, 2019

Choose a reason for hiding this comment

Zantares commented Sep 5, 2019

penpornk Sep 10, 2019

Choose a reason for hiding this comment

gbaned commented Sep 11, 2019

Zantares commented Sep 11, 2019 • edited

penpornk left a comment

Choose a reason for hiding this comment

penpornk Sep 11, 2019

Choose a reason for hiding this comment

Zantares Sep 23, 2019

Choose a reason for hiding this comment

penpornk commented Sep 11, 2019

Zantares commented Sep 23, 2019

penpornk left a comment

Choose a reason for hiding this comment

Zantares commented Sep 11, 2019 •

edited