Fixes in MKL add_n to address several regressions by agramesh1 · Pull Request #13021 · tensorflow/tensorflow

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

    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

+      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

+       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

      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

-
-    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

+        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 <[email protected]>: fixing build issue for this test -- e45b3b975c40b318ac4918859b364d8e8c3b304a by hmonishN <[email protected]>: fix build error -- 077b7a5b8277cb2e55b57aca64102871326afd0d by Harshit Monish <[email protected]>: 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 <[email protected]>: fixing build issue for this test -- e45b3b975c40b318ac4918859b364d8e8c3b304a by hmonishN <[email protected]>: fix build error -- 077b7a5b8277cb2e55b57aca64102871326afd0d by Harshit Monish <[email protected]>: 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 suggested 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 suggested 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

Conversation

agramesh1 commented Sep 13, 2017

Uh oh!

mention-bot commented Sep 13, 2017

Uh oh!

tensorflow-jenkins commented Sep 13, 2017

Uh oh!

drpngx commented Sep 13, 2017

Uh oh!

caisq commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmlarsen Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmlarsen commented Sep 20, 2017

Uh oh!

rmlarsen commented Sep 20, 2017

Uh oh!

rmlarsen commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rmlarsen Sep 20, 2017 •

edited

Loading