Remove use_c10_dispatcher: unboxed_only #36838

smessmer · 2020-04-17T22:28:58Z

Stack from ghstack:

Remove callBoxedWorkaround #36850 Remove callBoxedWorkaround
Remove use_c10_dispatcher: unboxed_only #36838 Remove use_c10_dispatcher: unboxed_only

All ops now do unboxing after dispatch, i.e. c10 handles unboxing and c10 registers a wrapper for the op to JIT
The last op that manually registered its own wrapper to JIT in register_aten_ops.cpp was migrated.

Since there are no ops using use_c10_dispatcher: unboxed_only anymore, we can delete the feature.

Also:

Rename some files to more accurately describe what they're doing now:
- OpsAlreadyMovedToC10.h/cpp -> ATenOpList.h/cpp
- register_aten_ops.cpp -> generated_unboxing_wrappers.cpp
- gen_jit_dispatch.py -> gen_unboxing_wrappers.py

Differential Revision: D21100081

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

All ops now do unboxing after dispatch, i.e. c10 handles unboxing and c10 registers a wrapper for the op to JIT The last op that manually registered its own wrapper to JIT in register_aten_ops.cpp was migrated. Since there are no ops using use_c10_dispatcher: unboxed_only anymore, we can delete the feature. Also: - Rename some files to more accurately describe what they're doing now: - OpsAlreadyMovedToC10.h/cpp -> ATenOpList.h/cpp - register_aten_ops.cpp -> generated_unboxing_wrappers.cpp - gen_jit_dispatch.py -> gen_unboxing_wrappers.cpp Differential Revision: [D21100081](https://our.internmc.facebook.com/intern/diff/D21100081/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21100081/)! [ghstack-poisoned]

All ops now do unboxing after dispatch, i.e. c10 handles unboxing and c10 registers a wrapper for the op to JIT The last op that manually registered its own wrapper to JIT in register_aten_ops.cpp was migrated. Since there are no ops using use_c10_dispatcher: unboxed_only anymore, we can delete the feature. Also: - Rename some files to more accurately describe what they're doing now: - OpsAlreadyMovedToC10.h/cpp -> ATenOpList.h/cpp - register_aten_ops.cpp -> generated_unboxing_wrappers.cpp - gen_jit_dispatch.py -> gen_unboxing_wrappers.cpp Differential Revision: [D21100081](https://our.internmc.facebook.com/intern/diff/D21100081/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21100081/)! ghstack-source-id: 102407222 Pull Request resolved: #36838

dr-ci · 2020-04-17T22:51:54Z

💊 Build failures summary and remediations

As of commit dd12431 (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 on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 10 times.

ezyang · 2020-04-20T16:31:32Z

Do we still need impl_UNBOXED?

smessmer · 2020-04-20T16:49:02Z

Do we still need impl_UNBOXED?

Yes, to get rid of that, we first need to fix all the gaps in #32366.

The new default (after this PR but without fixing those gaps) is 'with_codegenerated_unboxing_wrappers', i.e. if an op doesn't explicitly specify 'use_c10_dispatcher: full', it will get a codegenerated unboxing wrapper. The registration then still needs to use impl_UNBOXED so it doesn't generate a templated unboxing wrapper for it.

ezyang · 2020-04-20T18:14:48Z

aten/src/ATen/core/ATenOpList.h

+namespace at {
+
+// list of ATen ops that come from native_functions.yaml
+CAFFE2_API bool is_aten_op(const c10::OperatorName& opName);


This new name doesn't seem very descriptive. (A better question to ask, perhaps: why do I care if it is_aten_op? Why isn't this the same thing as just checking the namespace?)

Agreed, the implications of this name aren't that clear unless you're pretty deep into what's going on. Judging by its use here, I'd suggest maybe inverting the sense and making it is_custom_op instead.

checking the namespace is a good idea, we should move towards doing that instead. I added a TODO. It likely doesn't work just yet because we might have internal "custom" ops that use the aten namespace. For now, I renamed it to is_custom_op.

@smessmer when I get everything onto the new API, we will be able to guarantee that custom ops don't use the aten namespace.

But I am kind of curious what the downstream usage of this is, because the way I'm likely going to make the internal "Custom" ops (you're talking about the JIT ops right?) live in a centralized location is by hardcoding their registrations in TypeDefault.cpp ala #36925

See also #36926

ezyang

This seems like a strict improvement (so I will approve) but I don't see why you need the is_aten_op predicate anymore.

bhosmer

If @ezyang is right and is_aten_op can be replaced by a simple namespace check at the remaining callsites, that definitely seems better. Otherwise I'd vote for renaming per inline comment.

Also, not for this PR but in general I'd mildly prefer having the unboxed_only removal and the renaming be separate diffs, esp given that the file renaming ripples into the build scripts and so on.

bhosmer · 2020-04-20T20:36:59Z

aten/src/ATen/core/ATenOpList.h

+namespace at {
+
+// list of ATen ops that come from native_functions.yaml
+CAFFE2_API bool is_aten_op(const c10::OperatorName& opName);


Agreed, the implications of this name aren't that clear unless you're pretty deep into what's going on. Judging by its use here, I'd suggest maybe inverting the sense and making it is_custom_op instead.

bhosmer · 2020-04-20T20:39:55Z

torch/csrc/jit/runtime/register_c10_ops.cpp

@@ -169,16 +169,11 @@ class RegistrationListener final : public c10::OpRegistrationListener {
      // TODO Find a better way to handle this.
      return;
    }
-    if (at::is_aten_op_and_unboxing_is_already_handled_by_c10(
-            op.schema().operator_name())) {
+    if (at::is_aten_op(op.schema().operator_name())) {


Aside: in a world where the listener setup is no longer needed (because the JIT knows how to do c10 registry lookups directly), where will this conditional creation of a testing wrapper wind up?

Excellent question. I guess it would have to be in the call path which sucks. Moving this to do a namespace check instead might solve the issue.

All ops now do unboxing after dispatch, i.e. c10 handles unboxing and c10 registers a wrapper for the op to JIT The last op that manually registered its own wrapper to JIT in register_aten_ops.cpp was migrated. Since there are no ops using use_c10_dispatcher: unboxed_only anymore, we can delete the feature. Also: - Rename some files to more accurately describe what they're doing now: - OpsAlreadyMovedToC10.h/cpp -> ATenOpList.h/cpp - register_aten_ops.cpp -> generated_unboxing_wrappers.cpp - gen_jit_dispatch.py -> gen_unboxing_wrappers.cpp Differential Revision: [D21100081](https://our.internmc.facebook.com/intern/diff/D21100081/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21100081/)! [ghstack-poisoned]

facebook-github-bot · 2020-04-21T22:15:56Z

This pull request has been merged in 6d13a33.

smessmer requested a review from apaszke as a code owner April 17, 2020 22:28

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 17, 2020

smessmer mentioned this pull request Apr 17, 2020

Remove hacked twins from codegen #36666

Closed

smessmer requested a review from bhosmer April 17, 2020 22:34

smessmer mentioned this pull request Apr 18, 2020

Remove callBoxedWorkaround #36850

Closed

smessmer requested review from EscapeZero and ezyang and removed request for EscapeZero April 18, 2020 00:28

ezyang reviewed Apr 20, 2020

View reviewed changes

ezyang approved these changes Apr 20, 2020

View reviewed changes

bhosmer approved these changes Apr 20, 2020

View reviewed changes

smessmer added 3 commits April 20, 2020 14:12

facebook-github-bot closed this in 6d13a33 Apr 21, 2020

facebook-github-bot added the merged label Apr 21, 2020

facebook-github-bot deleted the gh/smessmer/206/head branch April 25, 2020 14:16

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove use_c10_dispatcher: unboxed_only #36838

Remove use_c10_dispatcher: unboxed_only #36838

smessmer commented Apr 17, 2020 •

edited

Loading

dr-ci bot commented Apr 17, 2020 •

edited

Loading

ezyang commented Apr 20, 2020

smessmer commented Apr 20, 2020 •

edited

Loading

ezyang Apr 20, 2020

bhosmer Apr 20, 2020

smessmer Apr 20, 2020 •

edited

Loading

ezyang Apr 21, 2020

ezyang left a comment

bhosmer left a comment

bhosmer Apr 20, 2020

bhosmer Apr 20, 2020

smessmer Apr 20, 2020 •

edited

Loading

facebook-github-bot commented Apr 21, 2020

Remove use_c10_dispatcher: unboxed_only #36838

Remove use_c10_dispatcher: unboxed_only #36838

Conversation

smessmer commented Apr 17, 2020 • edited Loading

dr-ci bot commented Apr 17, 2020 • edited Loading

💊 Build failures summary and remediations

ezyang commented Apr 20, 2020

smessmer commented Apr 20, 2020 • edited Loading

ezyang Apr 20, 2020

Choose a reason for hiding this comment

bhosmer Apr 20, 2020

Choose a reason for hiding this comment

smessmer Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

ezyang Apr 21, 2020

Choose a reason for hiding this comment

ezyang left a comment

Choose a reason for hiding this comment

bhosmer left a comment

Choose a reason for hiding this comment

bhosmer Apr 20, 2020

Choose a reason for hiding this comment

bhosmer Apr 20, 2020

Choose a reason for hiding this comment

smessmer Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Apr 21, 2020

smessmer commented Apr 17, 2020 •

edited

Loading

dr-ci bot commented Apr 17, 2020 •

edited

Loading

smessmer commented Apr 20, 2020 •

edited

Loading

smessmer Apr 20, 2020 •

edited

Loading

smessmer Apr 20, 2020 •

edited

Loading