Add OptionalRef; clamp: port to structured kernel
#61361
Conversation
For more information, see #55070. [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 6bc0599 (more details on the Dr. CI page and at hud.pytorch.org/pr/61361):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
For more information, see #55070. [ghstack-poisoned]
For more information, see #55070. [ghstack-poisoned]
|
@SplitInfinity sorry just wondering: is this PR stalled? I'm kind of blocked on it (need out variant of https://github.com/pytorch/pytorch/pull/61361/checks?check_run_id=3022080451#step:5:15930 but I don't think you've changed any macros that are used in that file. |
|
It is not stalled, I'm going to iterate on it and hopefully get all tests to pass today after submitting PSC. |
great. lmk if you need help getting it across the finish line |
For more information, see #55070. [ghstack-poisoned]
c10/core/Scalar.h
Outdated
| } | ||
|
|
||
| private: | ||
| const Scalar empty_; |
There was a problem hiding this comment.
The purpose of this member is so that scalar_ has something to refer to when the OptionalScalarRef instance refers to nothing. I tried to make it static in order to ensure there is only one instance per class and not per object, but I ran into linker errors I couldn't fix. I put the definition of said static member into Scalar.cpp and I suspect it there are some objects that include Scalar.h that aren't linked against whatever library Scalar.cpp ends up in. I dug through CMakeLists/Bazel files for a while but couldn't figure out how to fix this.
A fix for the above or another way to create something for scalar_ to refer to when the instance of OptionalScalarRef refers to nothing would be appreciated. 🙏
There was a problem hiding this comment.
Since your fields here are private, it seems better to just store a Scalar*. Then we don't even need HAS_n.
clamp: port to structured kernelclamp: port to structured kernel
|
This looks substantively fine but seeing how the OptionalScalarRef implementation turned out I'm retracting my suggestion to you that a |
| Tensor temp(result); | ||
| at::clamp_max_outf(self, max.toScalar(), temp); | ||
| result.copy_(temp); |
There was a problem hiding this comment.
This is another snippet I wanted to talk about. I would like to pass in result directly, but can't do that since the last parameter of at::clamp_max_outf is not const. None of the at::cpu:clamp_max variants have a const out parameter either...
There was a problem hiding this comment.
This kind of looks like #50953, but the function here is clamp_max, not clamp.
There was a problem hiding this comment.
(Don't Tensor temp(result), that's an unnecessary refcount bump). In the terminal state clamp_min_outf takes a const argument and the const cast is unnecessary)
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
c10/util/Optional.h
Outdated
| @@ -587,13 +587,16 @@ class optional : private OptionalBase<T> { | |||
| constexpr bool initialized() const noexcept { | |||
| return OptionalBase<T>::initialized(); | |||
| } | |||
|
|
|||
| public: | |||
There was a problem hiding this comment.
I had to make dataptr() public now that OptionalScalarRef stores const Scalar*. But it doesn't seem right to do this.
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
|
Rerequesting review because switching to |
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
c10/core/Scalar.h
Outdated
|
|
||
| operator bool() const { | ||
| return has_value(); | ||
| } |
There was a problem hiding this comment.
btw, if you hate optional's conversion to boolean, you could delete this implicit conversion and force people to write has_value() explicitly.
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
| @@ -60,7 +60,7 @@ void clamp_kernel_impl(TensorIterator& iter) { | |||
| }); | |||
| } | |||
|
|
|||
| void clamp_scalar_kernel_impl(TensorIterator& iter, Scalar min, Scalar max) { | |||
| void clamp_scalar_kernel_impl(TensorIteratorBase& iter, Scalar min, Scalar max) { | |||
There was a problem hiding this comment.
Hold on, shouldn't these be Scalar& to avoid copying Scalars around?
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. [ghstack-poisoned]
|
@SplitInfinity has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
c10/core/OptionalRef.h
Outdated
| class OptionalRef { | ||
| public: | ||
| OptionalRef() : data_(nullptr) {} | ||
| OptionalRef(const T* data) : data_(data) { |
There was a problem hiding this comment.
I'd probably just get rid of this constructor at this point.
clamp: port to structured kernelclamp: port to structured kernel
This PR ports the `clamp` kernel to the structured format. In addition, it introduces `OptionalScalarRef` as a replacement for `c10::optional<Scalar>&`. The latter, although it is a reference type, can still involve copying the contained `Scalar` (e.g. if the actual parameter is a `Scalar` or if a `c10::optional<Scalar>` is constructed just to call a kernel). `OptionalScalarRef` contains only a `const Scalar&`, and stores flag about whether the instance contains something inside the `Scalar` itself using a new tag. For more information, see #55070. Differential Revision: [D29821533](https://our.internmc.facebook.com/intern/diff/D29821533) [ghstack-poisoned]
|
@SplitInfinity merged this pull request in 1d2ea76. |
Stack from ghstack:
clamp: port to structured kernel #61361This PR ports the
clampkernel to the structured format. In addition, it introducesOptionalScalarRefas a replacement forc10::optional<Scalar>&. The latter, although it is a reference type, can still involve copying the containedScalar(e.g. if the actual parameter is aScalaror if ac10::optional<Scalar>is constructed just to call a kernel).OptionalScalarRefcontains only aconst Scalar&, and stores flag about whether the instance contains something inside theScalaritself using a new tag.For more information, see #55070.
Differential Revision: D29821533