Skip to content

Register List Kernels for DML #95

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 19, 2020
Merged

Register List Kernels for DML #95

merged 7 commits into from
Nov 19, 2020

Conversation

zhangxiang1993
Copy link
Member

No description provided.

@PatriceVignola
Copy link
Contributor

PatriceVignola commented Nov 17, 2020 •
edited by zhangxiang1993
Loading

Can you rename the PR to something that doesn't involve the branch name? Also don't forget to change the commit message when merging. #Resolved

PatriceVignola
PatriceVignola reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/tensor_array.h Outdated
@@ -17,6 +17,7 @@ limitations under the License.
#define TENSORFLOW_CORE_KERNELS_TENSOR_ARRAY_H_

#include <limits.h>

Copy link
Contributor

@PatriceVignola PatriceVignola Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

nit: extra line #Resolved

PatriceVignola
PatriceVignola reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.cc Outdated
@@ -33,6 +31,7 @@ limitations under the License.
#include "tensorflow/core/lib/core/coding.h"
#include "tensorflow/core/lib/core/errors.h"
#include "tensorflow/core/util/util.h"
#include "third_party/eigen3/unsupported/Eigen/CXX11/Tensor" #include "tensorflow/core/framework/op_kernel.h"
Copy link
Contributor

@PatriceVignola PatriceVignola Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

Why has it been modified? #Resolved

Copy link
Contributor

@jstoecker jstoecker Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

Mistake? Looks like you joined the two lines together. :) #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

It was modified by the clang-format. But no change here, so I reverted.

In reply to: 524980697 [](ancestors = 524980697)

PatriceVignola
PatriceVignola reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h Outdated
}
}
auto output_flat = output->shaped<T, 2>({1, output->NumElements()});
// auto output_flat = output->shaped<T, 2>({1, output->NumElements()});
Copy link
Contributor

@PatriceVignola PatriceVignola Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

Dead code? #Resolved

PatriceVignola
PatriceVignola reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h Outdated
Comment on lines 783 to 784
// OP_REQUIRES_OK(c, c->allocate_temp(tmp.dtype(), tmp.shape(),
// &aligned));
Copy link
Contributor

@PatriceVignola PatriceVignola Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

Dead code? #Resolved

@jstoecker jstoecker requested a review from adtsai November 17, 2020 17:23
jstoecker
jstoecker reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@jstoecker jstoecker left a comment

Choose a reason for hiding this comment

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

@adtsai needs to look at this. The coupling of tensor_array with list_kernels is a little concerning.

tensorflow/core/kernels/list_kernels.cc Outdated
@@ -33,6 +31,7 @@ limitations under the License.
#include "tensorflow/core/lib/core/coding.h"
#include "tensorflow/core/lib/core/errors.h"
#include "tensorflow/core/util/util.h"
#include "third_party/eigen3/unsupported/Eigen/CXX11/Tensor" #include "tensorflow/core/framework/op_kernel.h"
Copy link
Contributor

@jstoecker jstoecker Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

Mistake? Looks like you joined the two lines together. :) #Resolved

@zhangxiang1993 zhangxiang1993 changed the title User/xianz/listkernels 2 Register List Kernels for DML Nov 17, 2020
@zhangxiang1993 zhangxiang1993 force-pushed the user/xianz/listkernels_2 branch from d48500e to 0bd88ad Compare November 17, 2020 17:54
zhangxiang1993
zhangxiang1993 commented Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h Outdated
@@ -798,7 +764,7 @@ class TensorListFromTensor : public OpKernel {
TensorShape output_shape(t.shape());
output_shape.RemoveDim(0);
OP_REQUIRES(c, element_shape.IsCompatibleWith(output_shape),
errors::InvalidArgument(
errors::InvalidArgument(s
Copy link
Member Author

@zhangxiang1993 zhangxiang1993 Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

s [](start = 40, length = 1)

added by mistake, will be reverted #Resolved

adtsai
adtsai reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h Outdated
tmp.unaligned_flat<T>();
output_list.tensors().emplace_back(aligned);
tensor_array::TensorCopyUnaligned<Device, T>(c, &tmp, &aligned);
output_list.tensors().emplace_back(tmp);
Copy link
Contributor

@adtsai adtsai Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

tmp [](start = 41, length = 3)

Why was this changed? #Resolved

adtsai
adtsai reviewed Nov 17, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h Outdated
@@ -664,9 +642,8 @@ class TensorListSplit : public OpKernel {
// prevent this.
Tensor aligned;
OP_REQUIRES_OK(c, c->allocate_temp(tmp.dtype(), tmp.shape(), &aligned));
Copy link
Contributor

@adtsai adtsai Nov 17, 2020 •
edited by zhangxiang1993
Loading

Choose a reason for hiding this comment

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

allocate_temp [](start = 27, length = 13)

It looks like TensorCopyUnaligned already calls allocate_temp. Do callers need to allocate their own tensors, or not? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

The callers need. Removed the extra line to call allocate_temp.

In reply to: 525578893 [](ancestors = 525578893)

@zhangxiang1993
Copy link
Member Author

Thanks for the remind:>

In reply to: 728780643 [](ancestors = 728780643)

adtsai
adtsai reviewed Nov 19, 2020
View reviewed changes
tensorflow/core/kernels/list_kernels.h
ConstMatrixVector inputs_flat;
inputs_flat.reserve(indices.NumElements());
// ConstMatrixVector inputs_flat;
std::vector<PersistentTensor> values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code?

adtsai
adtsai approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@adtsai adtsai left a comment

Choose a reason for hiding this comment

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

:shipit:

@zhangxiang1993 zhangxiang1993 merged commit 444e427 into directml Nov 19, 2020
@PatriceVignola PatriceVignola deleted the user/xianz/listkernels_2 branch December 14, 2020 21:41
jstoecker pushed a commit that referenced this pull request Dec 15, 2020
@zhangxiang1993 @jstoecker
Register List Kernels for DML (#95)
c486460
jstoecker added a commit that referenced this pull request Dec 15, 2020
@jstoecker
Merge fixes from directml branch (#124)
c77b197 
Merges some of the recent changes from the directml branch:
* Use compute queue for AMD devices (#102)
* Register List Kernels for DML (#95)
* Update DirectMLX to latest (#104)
* Remove extra rows from test email (#106)
* Fix DML's Select kernel for int64 (#113)
* Fix list kernels and tensor array ops registration (#114)
* Simplify CI scripts (#112)
* Fix StridedSlice's input size coalescing (#115)
* Disable int64 image test (#116)
* Fix network share copy path (#117)
* Pipeline should continue if a test job fails (#118)
* Switch network share path to use build number instead of build ID
* Add missing HostMemory int32 registrations for _Arg and _RetVal (#122)
* Implement all the arithmetic Scatter and ResourceScatter operators (#121)
* Register emulated kernel implementations for RandomStandardNormal and TruncatedNormal (#120)
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jstoecker jstoecker jstoecker left review comments

@PatriceVignola PatriceVignola PatriceVignola left review comments

@adtsai adtsai adtsai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhangxiang1993 @PatriceVignola @jstoecker @adtsai