Fixes in MKL add_n to address several regressions #13021

agramesh1 · 2017-09-13T18:19:13Z

Fixed failures due to scalar and empty tensors

mention-bot · 2017-09-13T18:19:15Z

@agramesh1, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jinghuangintel to be a potential reviewer.

tensorflow-jenkins · 2017-09-13T18:19:15Z

Can one of the admins verify this patch?

drpngx · 2017-09-13T18:24:48Z

@rmlarsen could you take a look?

caisq · 2017-09-19T13:50:53Z

Gentle ping, @rmlarsen

rmlarsen · 2017-09-19T22:50:43Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

@@ -54,17 +54,63 @@ class MklAddNOp : public OpKernel {
    GetMklShape(ctx, 1, &(mkl_context.input2_shape));
    bool input2_in_mkl_format = mkl_context.input2_shape.IsMklTensor();

+    // handle the case of a scalar
+    if (!input1_in_mkl_format && !input0.dims()) {


prefer input0.dims() == 0.

rmlarsen · 2017-09-19T22:52:36Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

+      mkl_context.output_shape.SetMklTensor(false);
+      AllocateOutputSetMklShape(ctx, 0, &out_tensor, o_shape,
+                                mkl_context.output_shape);
+      void* out_o = const_cast<void*>(static_cast<const void*>(


You can use tensor->scalar()() to access the scalar value (both lvalue and rvalue), instead of all the ugly casting.

rmlarsen · 2017-09-19T22:57:13Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

+       Tensor* out_tensor = nullptr;
+       mkl_context.output_shape.SetMklTensor(false);
+       AllocateOutputSetMklShape(ctx, 0, &out_tensor, o_shape,
+                                mkl_context.output_shape);


rmlarsen · 2017-09-19T23:00:35Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

@@ -246,15 +259,15 @@ class MklAddNOp : public OpKernel {
      bool input1_in_mkl_format = input1_shape.IsMklTensor();
      bool input2_in_mkl_format = input2_shape.IsMklTensor();
      dnnDelete_F32(Eltwise);
+      if (!input1_in_mkl_format || !input2_in_mkl_format) {


This condition should be

if (input1_in_mkl_format || input2_in_mkl_format) {

To be the opposite of line 81.

@agramesh1 are you going to address this one? I think you'll leak memory otherwise.

@rmlarsen this part is the opposite of line 173 and 182, only in the case that input 1 and input 2 are not in mkl format, we will need to allocate the array. so we will release the buffer only in the case these two are not in mkl format. Thank you ~ please let me know if you have any suggestions. :)

haha, I guess I didn't look at the fully expanded view :-) Thanks for pointing that out.

rmlarsen · 2017-09-19T23:02:07Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

-
-    size_t* in2_sizes;
-    size_t* in2_strides;
+    size_t* in_sizes;


set both to nullptr either here or in initializer list of constructor.

rmlarsen

Please address previous comments.

rmlarsen · 2017-09-20T15:47:54Z

tensorflow/core/kernels/mkl_aggregate_ops.cc

-       AllocateOutputSetMklShape(ctx, 0, &out_tensor, o_shape,
+        Tensor* out_tensor = nullptr;
+        mkl_context.output_shape.SetMklTensor(false);
+        AllocateOutputSetMklShape(ctx, 0, &out_tensor, o_shape,
                                mkl_context.output_shape);


Still missing an extra indent in line 85.

rmlarsen · 2017-09-20T15:49:09Z

@tensorflow-jenkins test this please

rmlarsen · 2017-09-20T18:10:46Z

@tensorflow-jenkins test this please

rmlarsen · 2017-09-20T21:53:15Z

@agramesh1 thanks for the improvement.

Imported from GitHub PR openxla/xla#13021 replaces EXPECT_OK with TF_EXPECT_OK and added relevant header file and dependency. Error: #10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:134:3: error: use of undeclared identifier 'EXPECT_OK' [1792](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1792)#10 1431.9 134 | EXPECT_OK(dense_module); [1793](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1793)#10 1431.9 | ^ [1794](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1794)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:137:3: error: use of undeclared identifier 'EXPECT_OK' [1795](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1795)#10 1431.9 137 | EXPECT_OK(dense_result); [1796](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1796)#10 1431.9 | ^ [1797](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1797)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:158:3: error: use of undeclared identifier 'EXPECT_OK' [1798](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1798)#10 1431.9 158 | EXPECT_OK(sparse_module); [1799](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1799)#10 1431.9 | ^ [1800](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1800)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:161:3: error: use of undeclared identifier 'EXPECT_OK' [1801](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1801)#10 1431.9 161 | EXPECT_OK(sparse_result); [1802](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1802)#10 1431.9 | ^ Copybara import of the project: -- a7d07b02230794d9c13175b39473666d5d067c54 by hmonishN <hmonish@nvidia.com>: fixing build issue for this test -- e45b3b975c40b318ac4918859b364d8e8c3b304a by hmonishN <hmonish@nvidia.com>: fix build error -- 077b7a5b8277cb2e55b57aca64102871326afd0d by Harshit Monish <143435143+hmonishN@users.noreply.github.com>: Incorporated review comment. Merging this change closes #13021 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#13021 from hmonishN:hmonish/fix_gpu_sparse_dot_test_build_issue 077b7a5b8277cb2e55b57aca64102871326afd0d PiperOrigin-RevId: 636702747

Imported from GitHub PR openxla/xla#13021 replaces EXPECT_OK with TF_EXPECT_OK and added relevant header file and dependency. Error: #10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:134:3: error: use of undeclared identifier 'EXPECT_OK' [1792](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1792)#10 1431.9 134 | EXPECT_OK(dense_module); [1793](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1793)#10 1431.9 | ^ [1794](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1794)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:137:3: error: use of undeclared identifier 'EXPECT_OK' [1795](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1795)#10 1431.9 137 | EXPECT_OK(dense_result); [1796](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1796)#10 1431.9 | ^ [1797](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1797)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:158:3: error: use of undeclared identifier 'EXPECT_OK' [1798](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1798)#10 1431.9 158 | EXPECT_OK(sparse_module); [1799](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1799)#10 1431.9 | ^ [1800](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1800)#10 1431.9 xla/service/gpu/tests/gpu_sparse_dot_test.cc:161:3: error: use of undeclared identifier 'EXPECT_OK' [1801](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1801)#10 1431.9 161 | EXPECT_OK(sparse_result); [1802](https://gitlab-master.nvidia.com/dl/openxla/ci/-/jobs/94442414#L1802)#10 1431.9 | ^ Copybara import of the project: -- a7d07b02230794d9c13175b39473666d5d067c54 by hmonishN <hmonish@nvidia.com>: fixing build issue for this test -- e45b3b975c40b318ac4918859b364d8e8c3b304a by hmonishN <hmonish@nvidia.com>: fix build error -- 077b7a5b8277cb2e55b57aca64102871326afd0d by Harshit Monish <143435143+hmonishN@users.noreply.github.com>: Incorporated review comment. Merging this change closes #13021 PiperOrigin-RevId: 636832662

Fixes in add_n to address several regressions

74fa12d

googlebot added the cla: yes label Sep 13, 2017

drpngx requested a review from rmlarsen September 13, 2017 18:25

drpngx added the awaiting review Pull request awaiting review label Sep 13, 2017

caisq assigned rmlarsen Sep 19, 2017

rmlarsen reviewed Sep 19, 2017

View reviewed changes

rmlarsen requested changes Sep 19, 2017

View reviewed changes

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

minor modifications

cf8be73

rmlarsen requested changes Sep 20, 2017

View reviewed changes

fixes for bad indent

2b199df

rmlarsen approved these changes Sep 20, 2017

View reviewed changes

rmlarsen added awaiting testing (then merge) and removed stat:awaiting response Status - Awaiting response from author labels Sep 20, 2017

rmlarsen merged commit 8f27889 into tensorflow:master Sep 20, 2017

nammbash deleted the mkl_addn_fixes branch April 24, 2020 22:21

copybara-service bot mentioned this pull request May 23, 2024

PR #13021: Fix build issue of gpu_sparse_dot_test #68566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes in MKL add_n to address several regressions #13021

Fixes in MKL add_n to address several regressions #13021

agramesh1 commented Sep 13, 2017

mention-bot commented Sep 13, 2017

tensorflow-jenkins commented Sep 13, 2017

drpngx commented Sep 13, 2017

caisq commented Sep 19, 2017

rmlarsen Sep 19, 2017

rmlarsen Sep 19, 2017

rmlarsen Sep 19, 2017

rmlarsen Sep 19, 2017

rmlarsen Sep 20, 2017 •

edited

Loading

jinghuangintel Sep 20, 2017

rmlarsen Sep 20, 2017

rmlarsen Sep 19, 2017

rmlarsen left a comment

rmlarsen Sep 20, 2017

rmlarsen commented Sep 20, 2017

rmlarsen commented Sep 20, 2017

rmlarsen commented Sep 20, 2017

Fixes in MKL add_n to address several regressions #13021

Fixes in MKL add_n to address several regressions #13021

Conversation

agramesh1 commented Sep 13, 2017

mention-bot commented Sep 13, 2017

tensorflow-jenkins commented Sep 13, 2017

drpngx commented Sep 13, 2017

caisq commented Sep 19, 2017

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

rmlarsen Sep 20, 2017 • edited Loading

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

rmlarsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmlarsen commented Sep 20, 2017

rmlarsen commented Sep 20, 2017

rmlarsen commented Sep 20, 2017

rmlarsen Sep 20, 2017 •

edited

Loading