ATen DerivedType is dead, long live ATen RegisterDispatchKey by ezyang · Pull Request #47011 · pytorch/pytorch

ezyang · 2020-10-28T19:46:44Z

Stack from ghstack:

Structured kernel definitions #45277 Structured kernel definitions
ATen DerivedType is dead, long live ATen RegisterDispatchKey #47011 ATen DerivedType is dead, long live ATen RegisterDispatchKey

smessmer has complained about how it is difficult to find generated
code. Well hopefully this diffs helps a bit with that.

There are three components to this refactor:

Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU).
The 'Type' nomenclature is vestigial and I think Register says
what these files do a lot more clearly. I also got rid of
the CPUType namespace; everything just goes in anonymous
namespace now, less moving parts this way.
Give Math and DefaultBackend their own files (RegisterMath and
RegisterDefaultBackend)
Restructure code generation so that schema definition is done
completely separately from RegisterDispatchKey

I decided to name the files RegisterCPU rather than the old convention
BackendSelectRegister, because it seems better to me if these
files clump together in an alphabetical listing rather than being
spread out everywhere. There are a few manual registration files
which should probably get similar renaming.

I also did a little garden cleaning about how we identify if a
dispatch key is a cuda key or a generic key (previously called
KEYWORD_ALL_BACKENDS but I like my naming better).

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D24600806

smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 22d3fd7 Pull Request resolved: #47011

ailzhang · 2020-10-28T20:49:00Z

tools/codegen/gen.py

-    backends = [
+    # NB: substrings in these dispatch keys matter, we do tests to see if
+    # a key contains, e.g., CUDA to classify it as a CUDA backend
+    dispatch_keys = [


Btw a quick comment since I ran into this PR ;)
I tried to use dispatch_keywords to reference the keys we support in native_functions.yaml to disambiguate from the real dispatch keys. One main confusion if we keep using dispatch_keys is developers would naturally think all dispatch keys are supported (like XLA/Autograd/Autocast etc) but in fact not all dispatch keys are supported in native_functions.yaml.

Ah, so it wasn't just a typo!

Here's a take: what if we hypothetically, we should support all dispatch keys in native_functions.yaml? Then dispatch key naming would be appropriate.

smessmer

thanks, I like the new file names better.

smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D24600806](https://our.internmc.facebook.com/intern/diff/D24600806) [ghstack-poisoned]

smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 8a19f48 Pull Request resolved: #47011

dr-ci · 2020-11-11T03:23:15Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.9-py3.6

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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 7 times.

bhosmer

Nice rationalization!

😍

Couple very minor things inline, heed as desired :)

bhosmer · 2020-11-11T02:18:23Z

BUILD.bazel

+        "aten/src/ATen/RegisterSparseCPU.cpp",
+        "aten/src/ATen/RegisterMath.cpp",
+        "aten/src/ATen/RegisterDefaultBackend.cpp",
+        "aten/src/ATen/RegisterSchema.cpp",


minor, but is it worth globbing these?

It's not worth because cmake and buck limitations on generated files means they can't be globbed.

bhosmer · 2020-11-11T07:35:41Z

tools/codegen/gen.py

+# Dispatch keys that "support all backends".  These codegen slightly differently
+# then backend specific keys.
+def is_generic_dispatch_key(dk: str) -> bool:
+    return dk in {'DefaultBackend', 'Math'}


Personally I'd rather retain a top-level definition for this than sink it into a helper like this (though adding a helper that tests it is fine by me) - feels fragmented to have part of the data model hidden in a function.

I'll leave this to future work

I'll leave this to future work

What, putting the top-level definition back? 😁

I probably wasn't clear - what I meant was, the function you added is fine by me, but I'd rather see it use the existing top-level definition, rather than capture it. Like:

GENERIC_DISPATCH_KEYS = ('DefaultBackend', 'Math') def is_generic_dispatch_key(dk: str) -> bool: return dk in GENERIC_DISPATCH_KEYS

Not a super strong opinion, just a personal pref. But wanted to clarify

Yeah, haha (because I had a clean land bill so it was ready to land).

I do understand the suggestion here... it just conflicts with another pref I have, which is default lots of encapsulation (hide everything by default) and then make it visible. But it's NBD, and there's more refactoring for Dispatch Keys that probably could be done (like making it into an enum)

smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D24600806](https://our.internmc.facebook.com/intern/diff/D24600806) [ghstack-poisoned]

facebook-github-bot · 2020-11-12T19:20:03Z

@ezyang merged this pull request in 809660f.

…#47011) Summary: Pull Request resolved: pytorch#47011 smessmer has complained about how it is difficult to find generated code. Well hopefully this diffs helps a bit with that. There are three components to this refactor: - Rename TypeDerived (CPUType) to RegisterDispatchKey (RegisterCPU). The 'Type' nomenclature is vestigial and I think Register says what these files do a lot more clearly. I also got rid of the CPUType namespace; everything just goes in anonymous namespace now, less moving parts this way. - Give Math and DefaultBackend their own files (RegisterMath and RegisterDefaultBackend) - Restructure code generation so that schema definition is done completely separately from RegisterDispatchKey I decided to name the files RegisterCPU rather than the old convention BackendSelectRegister, because it seems better to me if these files clump together in an alphabetical listing rather than being spread out everywhere. There are a few manual registration files which should probably get similar renaming. I also did a little garden cleaning about how we identify if a dispatch key is a cuda key or a generic key (previously called KEYWORD_ALL_BACKENDS but I like my naming better). Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: D24600806 Test Plan: Imported from OSS Reviewed By: smessmer Pulled By: ezyang fbshipit-source-id: c1b510dd7515bd95e3ad25b8edf961b2fb30a25a

ailzhang reviewed Oct 28, 2020

View reviewed changes

ezyang requested a review from smessmer October 29, 2020 20:51

facebook-github-bot added the cla signed label Oct 30, 2020

ezyang requested a review from bhosmer November 10, 2020 22:37

smessmer approved these changes Nov 10, 2020

View reviewed changes

bhosmer approved these changes Nov 11, 2020

View reviewed changes

This was referenced Nov 11, 2020

Simplify device guard code generation. #47765

Closed

Structured kernel definitions #45277

Closed

facebook-github-bot closed this in 809660f Nov 12, 2020

facebook-github-bot added the Merged label Nov 12, 2020

facebook-github-bot deleted the gh/ezyang/863/head branch November 16, 2020 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ATen DerivedType is dead, long live ATen RegisterDispatchKey#47011

ATen DerivedType is dead, long live ATen RegisterDispatchKey#47011
ezyang wants to merge 3 commits intogh/ezyang/863/basefrom
gh/ezyang/863/head

ezyang commented Oct 28, 2020 •

edited

Loading

Uh oh!

ailzhang Oct 28, 2020

Uh oh!

ezyang Oct 29, 2020

Uh oh!

smessmer left a comment

Uh oh!

dr-ci bot commented Nov 11, 2020 •

edited

Loading

Uh oh!

bhosmer left a comment

Uh oh!

bhosmer Nov 11, 2020

Uh oh!

ezyang Nov 11, 2020

Uh oh!

bhosmer Nov 11, 2020

Uh oh!

ezyang Nov 12, 2020

Uh oh!

bhosmer Nov 12, 2020 •

edited

Loading

Uh oh!

ezyang Nov 12, 2020

Uh oh!

facebook-github-bot commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ezyang commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ailzhang Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

smessmer left a comment

Choose a reason for hiding this comment

Uh oh!

dr-ci bot commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

bhosmer left a comment

Choose a reason for hiding this comment

Uh oh!

bhosmer Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

bhosmer Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

bhosmer Nov 12, 2020 • 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!

ezyang Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ezyang commented Oct 28, 2020 •

edited

Loading

dr-ci bot commented Nov 11, 2020 •

edited

Loading

bhosmer Nov 12, 2020 •

edited

Loading