[INTEL MKL] Enabled Conv2D fprop for MKL-DNN v1.0. #30549

bhavani-subramanian · 2019-07-09T23:57:24Z

No description provided.

penpornk

Thank you for your PR! I'm a little concerned with redundant codes. I think we should minimize them.

tensorflow/core/graph/mkl_layout_pass.cc

tensorflow/core/kernels/mkl_conv_ops.cc

penpornk · 2019-07-11T23:32:19Z

tensorflow/core/kernels/mkl_conv_ops.cc

+  void AllocatePersistentTensor(OpKernelContext* context,
+                                const ConvFwdPd& conv_prim_desc,
+                                Tensor** filter_tensor,
+                                const MklDnnShape& filter_mkl_shape) {


Please try to merge with existing original code (same or the rest of the PR). This one can call the one with three parameters then do extra processing.

tensorflowbutler · 2019-07-27T12:23:09Z

It has been 14 days with no activity and the awaiting response label was assigned. Is this PR still valid? Assigning the stalled label. Please comment to reassure me that this is still being worked on.

bhavani-subramanian · 2019-07-27T21:13:41Z

@tensorflowbutler yes, this PR is still being worked on. I'll push the requested changes within a couple of days. Thanks!

bhavani-subramanian · 2019-07-29T20:30:17Z

@penpornk I have addressed your review comments. Please take a look. Thanks!

penpornk

Thank you very much for all the changes! This looks great! I have a few more comments.

tensorflow/core/graph/mkl_layout_pass.cc

penpornk · 2019-07-29T21:02:27Z

tensorflow/core/kernels/mkl_conv_ops.cc

+        static_cast<int32>(
+            GetFilterTfDataFormat(filter_mkl_shape, conv_prim_desc));
+#else
+        GetFilterTfDataFormat(filter_mkl_shape, conv_prim_desc);


To be added.

penpornk · 2019-07-29T21:16:53Z

tensorflow/core/kernels/mkl_conv_ops.cc

+        static_cast<int32>(
+            GetFilterTfDataFormat(filter_mkl_shape, conv_prim_desc));
+#else
+        GetFilterTfDataFormat(filter_mkl_shape, conv_prim_desc);


This looks similar to the v1 case. static_cast<int32> shouldn't make a difference since we are assigning it to the int32 version of second_tensor anyway. Can we merge this?

penpornk · 2019-07-29T21:24:20Z

tensorflow/core/kernels/mkl_conv_ops.h

@@ -136,8 +138,13 @@ class MklDnnConvUtil {
      CHECK_BOUNDS(input_cols_raw, "Input cols too large");
      int input_cols = static_cast<int>(input_cols_raw);

+#ifdef ENABLE_MKLDNN_V1
+      // MKL-DNN always requires input in NCHW format Conv2D.
+      std::vector<long int> mkldnn_sizes(4, -1);


You can define MKLDNN_SIZE_DTYPE (or any name you prefer) to be either long int or int at the beginning and then use it throughout the file.

Suggested change

std::vector<long int> mkldnn_sizes(4, -1);

std::vector<MKLDNN_SIZE_DTYPE> mkldnn_sizes(4, -1);

bhavani-subramanian · 2019-07-29T22:38:05Z

@penpornk I have addressed your latest review comments. Please let me know if it looks okay. Thank you!

penpornk

Thank you for your patience! One more comment, please.

penpornk · 2019-07-29T22:52:28Z

tensorflow/core/kernels/mkl_conv_ops.h

@@ -658,6 +631,10 @@ class MklDummyOp : public OpKernel {
  }
 };

+#ifdef ENABLE_MKLDNN_V1
+#undef MKLDNN_SIZE_DTYPE


MKLDNN_SIZE_DTYPE is always defined, we don't need to guard it with v1 ifdef.

Yes, you're right :) Fixed it.

…ined.

penpornk

Thank you very much again! I think we're good to go. :)

PiperOrigin-RevId: 260989299

Enabled Conv2D fprop for MKL-DNN v1.0.

14b14ab

tensorflow-bot bot added the size:L CL Change Size: Large label Jul 9, 2019

googlebot added the cla: yes label Jul 9, 2019

rthadur requested a review from penpornk July 9, 2019 23:59

rthadur self-assigned this Jul 9, 2019

rthadur added this to Assigned Reviewer in PR Queue via automation Jul 9, 2019

rthadur added the comp:mkl MKL related issues label Jul 9, 2019

bhavani-subramanian added 2 commits July 10, 2019 10:27

Changed CHECK to DCHECK.

3608a97

Ran Clang format checks.

fdf9ee6

penpornk requested changes Jul 11, 2019

View reviewed changes

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jul 11, 2019

rthadur added the stat:awaiting response Status - Awaiting response from author label Jul 12, 2019

tensorflowbutler added the stale This label marks the issue/pr stale - to be closed automatically if no activity label Jul 27, 2019

tensorflowbutler added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting response Status - Awaiting response from author stale This label marks the issue/pr stale - to be closed automatically if no activity labels Jul 28, 2019

bhavani-subramanian added 4 commits July 29, 2019 11:26

Merge branch 'master' into mkldnn-1.0-conv2d-fwd

1b2a220

Addressed review comments.

7c0bcbe

Merge branch 'master' into mkldnn-1.0-conv2d-fwd

9cd5724

Merge branch 'master' into mkldnn-1.0-conv2d-fwd

1e3ac6a

rthadur requested a review from penpornk July 29, 2019 20:29

penpornk requested changes Jul 29, 2019

View reviewed changes

Addressed some more review comments.

cae3481

penpornk requested changes Jul 29, 2019

View reviewed changes

Removed ifdef guard for MKLDNN_SIZE_DTYPE since it will always be def…

ab4af31

…ined.

penpornk approved these changes Jul 29, 2019

View reviewed changes

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jul 29, 2019

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

kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 29, 2019

tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Jul 30, 2019

rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 30, 2019

tensorflow-copybara merged commit ab4af31 into tensorflow:master Jul 31, 2019

PR Queue automation moved this from Approved by Reviewer to Merged Jul 31, 2019

tensorflow-copybara pushed a commit that referenced this pull request Jul 31, 2019

Merge pull request #30549 from Intel-tensorflow:mkldnn-1.0-conv2d-fwd

644e458

PiperOrigin-RevId: 260989299

gzmkl mentioned this pull request Jan 10, 2020

[INTEL MKL] Refactor the implementation of forward conv ops w MKL-DNN v1.0 integration - #35757

Merged

This was referenced Jan 15, 2020

[INTEL MKL] Refactor the implementation of forward conv ops w MKL-DNN v1.0 integration #35907

Closed

MKL DNN 1.0 integration - refactoring MKL conv forward op #35908

Closed

nammbash deleted the mkldnn-1.0-conv2d-fwd branch April 25, 2020 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INTEL MKL] Enabled Conv2D fprop for MKL-DNN v1.0. #30549

[INTEL MKL] Enabled Conv2D fprop for MKL-DNN v1.0. #30549

bhavani-subramanian commented Jul 9, 2019

penpornk left a comment

penpornk Jul 11, 2019

bhavani-subramanian Jul 29, 2019

tensorflowbutler commented Jul 27, 2019

bhavani-subramanian commented Jul 27, 2019

bhavani-subramanian commented Jul 29, 2019

penpornk left a comment •

edited

penpornk Jul 29, 2019

bhavani-subramanian Jul 29, 2019

penpornk Jul 29, 2019

bhavani-subramanian Jul 29, 2019

penpornk Jul 29, 2019

bhavani-subramanian Jul 29, 2019

bhavani-subramanian commented Jul 29, 2019

penpornk left a comment

penpornk Jul 29, 2019

bhavani-subramanian Jul 29, 2019

penpornk left a comment

	std::vector<long int> mkldnn_sizes(4, -1);
	std::vector<MKLDNN_SIZE_DTYPE> mkldnn_sizes(4, -1);

[INTEL MKL] Enabled Conv2D fprop for MKL-DNN v1.0. #30549

[INTEL MKL] Enabled Conv2D fprop for MKL-DNN v1.0. #30549

Conversation

bhavani-subramanian commented Jul 9, 2019

penpornk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tensorflowbutler commented Jul 27, 2019

bhavani-subramanian commented Jul 27, 2019

bhavani-subramanian commented Jul 29, 2019

penpornk left a comment • edited

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

bhavani-subramanian commented Jul 29, 2019

penpornk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penpornk left a comment

Choose a reason for hiding this comment

penpornk left a comment •

edited