add native view_copy.out ops, teach codegen about tensorlist out=#76126
add native view_copy.out ops, teach codegen about tensorlist out=#76126bdhirsh wants to merge 18 commits intogh/bdhirsh/214/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful links
✅ No Failures (20 Pending)As of commit 0ceed57 (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
aten/src/ATen/native/TensorShape.cpp
Outdated
| } | ||
|
|
||
|
|
||
| at::ITensorListRef split_copy_Tensor_out(const at::Tensor & self, int64_t split_size, int64_t dim, at::TensorList out) { |
There was a problem hiding this comment.
@ezyang / @ysiraichi This PR isn't ready for review yet, but I'm wondering - what do you think the dispatcher return type should be for functions like this, with the following schema?
split_copy.Tensor_out(Tensor self, int split_size, int dim=0, *, Tensor(a!)[] out) -> Tensor(a!)[]
tldr is that this PR introduces some new ops that have a schema that the codegen hasn't had to deal with before - out= arguments that are lists of mutable tensors. I think the answer is that we want it to be ITensorListRef, but that currently breaks - so I wanted to clarify my reasoning.
Just to list out all the options:
(1) make the return type ITensorListRef
This breaks boxed_to_unboxed code right now, because we don't currently know how to convert IValue to ITensorListRef. I think... this is probably what we want? But it looks like this doesn't work today, so I was wondering what the current state of ITensorListRef is (or maybe I'm misunderstanding something - shouldn't you be able to convert an IValue to an ITensorListRef?)
(2) make the return type std::vector<Tensor>
This will work, but has two disadvantages: We have a bunch of codegen that expects the out= argument and the return type to be the same, so that we can just return out in the generated kernels. We'd have to modify those to explicitly convert the output to a vector (return out.vec()), and also accept the perf hit (I don't expect any of these kernels to be hotpath though).
(3) make the return type at::TensorList
This also breaks today, because IValues don't know how to convert to a TensorList (ArrayRef<Tensor>). I think it would also be... wrong, because we'd need to convert from the IValue's internal c10::List<Tensor> to ArrayRef<Tensor>, which requires creating new tensors to make them all contiguous in memory, but they aren't owned by anyone.
There was a problem hiding this comment.
I was just discussing this at #76049 We should make the return type void. This would be BC breaking but I doubt anyone is actually using this API and in any case they can't do method chaining like this.
…st out=" [ghstack-poisoned]
…st out=" [ghstack-poisoned]
| return BaseCType(scalarT) | ||
| elif isinstance(t, ListType): | ||
| elem = returntype_type(t.elem, mutable=mutable) | ||
| elem = returntype_type(t.elem, mutable=False) |
There was a problem hiding this comment.
maybe you want to assert that mutable is false here?
There was a problem hiding this comment.
yep, thanks! (this was a relic of me originally trying to return Tensor(a!)[] in the first version of the PR, but now these functions don't return anything).
tools/codegen/api/cpp.py
Outdated
| assert t.size is None, f"fixed size list returns not supported: {t}" | ||
| return VectorCType(elem) | ||
| if mutable and t.elem == BaseType(BaseTy.Tensor): | ||
| return BaseCType(iTensorListRefT) |
There was a problem hiding this comment.
it's weird to compute elem and then ignore it in this branch
…st out=" [ghstack-poisoned]
…st out=" [ghstack-poisoned]
…st out=" [ghstack-poisoned]
…st out=" [ghstack-poisoned]
|
unsubbing till this is ready for review |
|
yep, sorry about that (everything in this stack except this PR + the LTC prototype are either ready to be landed soon or ready for review - you've reviewed most of them though) |
…st out=" [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
|
PR should be ready for review now (letting CI run again, but I think I've fixed tests + lints) |
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
ezyang
left a comment
There was a problem hiding this comment.
With the assumption that these are getting gen'ed later.
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
|
@pytorchbot merge on green |
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
…st out=" This PR adds `out=` variants for all of the existing `view_copy` operators. I added them all manually in native_functions.yaml, and gave them implementations in `TensorShape.cpp` Our codegen required some changes to handle them properly, because this is the first time we've had native functions that are `out=`, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually be `void`, and not `Tensor(a!)[]` - because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something. [ghstack-poisoned]
|
@pytorchbot merge this please |
|
Hey @bdhirsh. |
…6126) Summary: Pull Request resolved: #76126 Approved by: https://github.com/ezyang Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/edc904d6ba50f8db2777fb1d60b3e7e074786760 Reviewed By: seemethere Differential Revision: D36494112 Pulled By: bdhirsh fbshipit-source-id: fa06de65f45bb1e5191d961a4b6a4dd1fb0e478b
This PR adds
out=variants for all of the existingview_copyoperators. I added them all manually in native_functions.yaml, and gave them implementations inTensorShape.cppOur codegen required some changes to handle them properly, because this is the first time we've had native functions that are
out=, with the out parameter being a list of tensors. As Ed pointed out in this issue #76049, the return type of these functions should actually bevoid, and notTensor(a!)[]- because you can't do method chaining on the output (since it's a list), then there's nothing useful about returning something.Stack from ghstack: