hacky_wrapper_for_legacy_signatures reorders out arguments by smessmer · Pull Request #48911 · pytorch/pytorch

smessmer · 2020-12-07T00:08:51Z

Stack from ghstack:

Remove .impl_UNBOXED() and functionalities associated with it #49220 Remove .impl_UNBOXED() and functionalities associated with it
Remove codegen logic to support non-c10-full ops #49164 Remove codegen logic to support non-c10-full ops
Making ops c10-full: list of optional tensors #49138 [wip] Making ops c10-full: list of optional tensors
Making ops c10-full: ConstQuantizerPtr #49150 Making ops c10-full: ConstQuantizerPtr
Making ops c10-full: Storage arguments #49146 Making ops c10-full: Storage arguments
Making ops c10-full: optional lists #49088 Making ops c10-full: optional lists
Making ops c10 full: optional out arguments #49083 Making ops c10 full: optional out arguments
Making ops c10-full: Generator arguments #49013 Making ops c10-full: Generator arguments
Making ops c10 full: out overloads with default arguments #49012 Making ops c10 full: out overloads with default arguments
Making ops c10-full: Dimname arguments #49008 Making ops c10-full: Dimname arguments
Making ops c10-full: ops blocked by manual registrations #49007 Making ops c10-full: ops blocked by manual registrations
Making ops c10 full: ops returning multiple out arguments #49006 Making ops c10 full: ops returning multiple out arguments
Make out ops c10-full (with hacky-wrapper) #48912 Make out ops c10-full (with hacky-wrapper)
hacky_wrapper_for_legacy_signatures reorders out arguments #48911 hacky_wrapper_for_legacy_signatures reorders out arguments

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten.

This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops.

Differential Revision: D25363336

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten. This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops. Differential Revision: [D25363336](https://our.internmc.facebook.com/intern/diff/D25363336/) [ghstack-poisoned]

dr-ci · 2020-12-07T00:29:38Z

💊 CI failures summary and remediations

As of commit c33786e (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 68 times.

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten. This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops. Differential Revision: [D25363336](https://our.internmc.facebook.com/intern/diff/D25363336/) [ghstack-poisoned]

ezyang · 2020-12-08T17:52:23Z

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h


 #include <c10/util/Metaprogramming.h>
 #include <c10/util/TypeList.h>
+#include <c10/util/IntegerSequence.h>


ngl: I think I would have preferred it if this was implemented via codegen rather than template

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten. This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops. Differential Revision: [D25363336](https://our.internmc.facebook.com/intern/diff/D25363336/) [ghstack-poisoned]

bhosmer

A few comments inline, but the only big question is whether it's really necessary to handle the general case of out args appearing in arbitrary positions in the signature - doing so makes things much much more complicated than they would need to be to handle the standard pattern.

On a quick search of native_functions.yaml I can't find any ops that actually break the standard pattern. I might have missed such, but if not, I'd like to take advantage by drastically simplifying both this code and the utilities in IntegerSequence.h.

(If there's a use case for the full capability, can you point me at it? I'd dearly love to find some other way of handling it. :)

[Editing to zoom out and note agreement with @ezyang that codegen feels like it would solve this problem more straightforwardly and without loss of generality (e.g. since we're servicing only native_functions ops, out-of-tree access to the functionality isn't an issue). But also worth noting that this path was settled on prior to the recent codegen refactor, which shifted the landscape in which the decision was made somewhat. Since the TTL of hacky_wrapper is quite finite, we've decided to land a template-based approach rather than retool.]

aten/src/ATen/core/boxing/impl/boxing.h

bhosmer · 2020-12-09T04:14:53Z

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

+struct with_out_arguments_reordered_impl final {
+private:
+    // For an example op
+    //   > aten::example(Tensor a, int64_t b, Tensor(a!) out_c, int64_t d, Tensor(b!) out_e) -> (Tensor(a!), Tensor(b!))


This example op has out arguments sprinkled through the signature, which AFAICT never happens in native_functions.yaml. I think it'd be clearer to use an example of the dominant pattern - out args at the end. (Among other things it would avoid the confusion of describing ops like this as having out arguments at the end, as on line 277.)

More importantly, are there any actual ops where out arguments aren't contiguous at the end? I think this logic (both here and in codegen, but especially here) could be made quite a bit simpler if it didn't have to handle that class of signatures.

You're right that the current implementation is more general than it would have to be. Not sure if restricting it to the actually occuring scenarios would simplify the logic though. Looking through it, the implementation could take two numbers [OutTensorArgIndicesBegin, OutTensorArgIndicesEnd] to represent a range instead of taking a full list of argument indices. But I think that's it, inside the metaprogramming I'd then still have to build the full OutTensorArgIndices array, so actually the metaprogramming would then contain an additional step and be more complex. Open to suggestions though if you have simplification ideas.

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

bhosmer · 2020-12-09T04:29:39Z

tools/codegen/gen.py

+c10::impl::hacky_wrapper_for_legacy_signatures<
+    {dispatcher_sig.type()},
+    std::index_sequence<{', '.join(mutable_argument_indices(f.func))}>>(TORCH_FN({sig.name()}))
+"""


Should be easy to avoid the copy pasted payload here and on 437 I hope - a little helper function rather than a CodeTemplate ideally

I'm following the style in the rest of the file where we duplicate and explicitly spell out generated code instead of putting it into subroutines, search for example for "findSchemaOrThrow". But I'm open to writing it either way, wdyt?

Actually, this is a misunderstanding of why gen_structured is copy-pasted from gen_unstructured: the goal is that structured kernels never have any of the legacy crud that is in unstructured.

Of course, the reality is more bleak; gen structured has to test for use_c10_dispatcher: full because it prior to this patch series, it literally does not work. But I feel like at least for hacky_wrapper_for_legacy_signatures we should be able to avoid this for structured kernels, right? :o)

@smessmer cool, house style works for me. But

@ezyang I could be misreading your comment, but are you thinking we just by fiat declare that a structured kernel/hacky wrapper combo is unsupported? That might be a nice inducement to finish kernel porting promptly, I guess.

Yes. There's only two ops that are structured and I can volunteer to do the port. Need to take a look.

bhosmer · 2020-12-09T04:30:20Z

tools/codegen/gen.py

                else:
-                    payload = f'torch::CppFunction::makeUnboxedOnly({sig.name()})'
+                    assert local.use_c10_dispatcher() is UseC10Dispatcher.with_codegenerated_unboxing_wrapper
+                    payload = f"torch::CppFunction::makeUnboxedOnly(&{sig.name()})"


Is this change necessitated by the hacky_wrapper stuff?

which change particularly? The added ampersand to get a pointer? No, that's a code style fix -- C++ is kind of inconsistent in that you can get function pointers without the ampersand but need it for getting pointers for anything else. Or do you mean the added assertion? That's just to make sure I handled all cases UseC10Dispatcher can take, just in case we add more states later (hopefully we don't).

I meant the ampersand :) thanks for the detail!

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten. This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops. Differential Revision: [D25363336](https://our.internmc.facebook.com/intern/diff/D25363336/) [ghstack-poisoned]

bhosmer

Cool! I think the simplified version is much clearer and easier to follow.

One remaining pitch to simplify even further - with a minor change, the functionality in #48910 isn't needed at all.
Also should make sure @ezyang's comment (re structured kernel support) isn't actionable before landing.

aten/src/ATen/core/op_registration/hacky_wrapper_for_legacy_signatures.h

bhosmer · 2020-12-11T02:34:12Z

tools/codegen/gen.py

                else:
-                    payload = f'torch::CppFunction::makeUnboxedOnly({sig.name()})'
+                    assert local.use_c10_dispatcher() is UseC10Dispatcher.with_codegenerated_unboxing_wrapper
+                    payload = f"torch::CppFunction::makeUnboxedOnly(&{sig.name()})"


I meant the ampersand :) thanks for the detail!

bhosmer · 2020-12-11T02:42:26Z

tools/codegen/gen.py

+c10::impl::hacky_wrapper_for_legacy_signatures<
+    {dispatcher_sig.type()},
+    std::index_sequence<{', '.join(mutable_argument_indices(f.func))}>>(TORCH_FN({sig.name()}))
+"""


@smessmer cool, house style works for me. But

@ezyang I could be misreading your comment, but are you thinking we just by fiat declare that a structured kernel/hacky wrapper combo is unsupported? That might be a nice inducement to finish kernel porting promptly, I guess.

This enables us to use hacky_wrapper_for_legacy_signatures for ops with out arguments so they can use templated unboxing logic without having to be rewritten. This only actually enables it for one op as a proof of concept. There will be a separate PR enabling it for more ops. Differential Revision: [D25363336](https://our.internmc.facebook.com/intern/diff/D25363336/) [ghstack-poisoned]

facebook-github-bot · 2020-12-11T09:11:44Z

This pull request has been merged in 56a157f.

facebook-github-bot added the cla signed label Dec 7, 2020

smessmer requested review from bhosmer and ezyang and removed request for bhosmer December 7, 2020 00:11

smessmer added 3 commits December 7, 2020 18:50

ezyang reviewed Dec 8, 2020

View reviewed changes

bhosmer reviewed Dec 9, 2020

View reviewed changes

smessmer requested a review from bhosmer December 9, 2020 08:29

This was referenced Dec 9, 2020

remove hacky_wrapper from BackendSelect #49079

Closed

Making ops c10 full: optional out arguments #49083

Closed

smessmer mentioned this pull request Dec 9, 2020

Making ops c10-full: optional lists #49088

Closed

smessmer added 3 commits December 9, 2020 19:03

smessmer mentioned this pull request Dec 10, 2020

Making ops c10-full: list of optional tensors #49138

Closed

This was referenced Dec 10, 2020

Making ops c10-full: Storage arguments #49146

Closed

Making ops c10-full: ConstQuantizerPtr #49150

Closed

smessmer added 2 commits December 9, 2020 23:16

smessmer mentioned this pull request Dec 10, 2020

Remove codegen logic to support non-c10-full ops #49164

Closed

bhosmer approved these changes Dec 11, 2020

View reviewed changes

smessmer added 2 commits December 10, 2020 21:53

smessmer mentioned this pull request Dec 11, 2020

Remove .impl_UNBOXED() and functionalities associated with it #49220

Closed

facebook-github-bot closed this in 56a157f Dec 11, 2020

facebook-github-bot added the Merged label Dec 11, 2020

facebook-github-bot deleted the gh/smessmer/271/head branch December 14, 2020 15:17

Conversation

smessmer commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Dec 7, 2020 • edited by facebook-github-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhosmer left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smessmer commented Dec 7, 2020 •

edited

Loading

dr-ci bot commented Dec 7, 2020 •

edited by facebook-github-bot

Loading

bhosmer left a comment •

edited

Loading