Desugar missing dispatch field into singleton Math entry by ezyang · Pull Request #46970 · pytorch/pytorch

ezyang · 2020-10-28T05:23:17Z

Stack from ghstack:

ATen DerivedType is dead, long live ATen RegisterDispatchKey #47011 ATen DerivedType is dead, long live ATen RegisterDispatchKey
Convert from higher order functions to classes in tools.codegen.gen #47008 Convert from higher order functions to classes in tools.codegen.gen
Delete TypeDefault.h and TypeDerived.h codegen entirely. #47002 Delete TypeDefault.h and TypeDerived.h codegen entirely.
Delete TypeDefault call code generation logic in VariableType #47000 Delete TypeDefault call code generation logic in VariableType
Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp #46991 Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp
Desugar missing dispatch field into singleton Math entry #46970 Desugar missing dispatch field into singleton Math entry
Some miscellaneous cleanup in codegen #46940 Some miscellaneous cleanup in codegen
Delete Vulkan from code generator. #46938 Delete Vulkan from code generator.

Now that catchall declarations are reinterpreted as registrations to
dispatch key Math, we can now simplify code generation logic by directly
generating to Math, and bypasing logic for catchall. This also helps
avoid bugs where we incorrectly classify some kernels as Math and others
as not, even though they get registered in the same way.

Bill of changes:

Give Math its own unique TORCH_LIBRARY_IMPL
Make it so NativeFunction.dispatch is always non-None. Simplify
downstream conditionals accordingly
When parsing NativeFunction, fill in missing dispatch with a
singleton Math entry (pointing to the cpp.name!)

One thing that is a little big about this change is a lot of kernels
which previously didn't report as "math" now report as math. I picked
a setting for these booleans that made sense to me, but I'm not sure
if e.g. XLA will handle it 100% correctly.

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

Differential Revision: D24592391

Now that catchall declarations are reinterpreted as registrations to dispatch key Math, we can now simplify code generation logic by directly generating to Math, and bypasing logic for catchall. This also helps avoid bugs where we incorrectly classify some kernels as Math and others as not, even though they get registered in the same way. Bill of changes: - Give Math its own unique TORCH_LIBRARY_IMPL - Make it so NativeFunction.dispatch is always non-None. Simplify downstream conditionals accordingly - When parsing NativeFunction, fill in missing dispatch with a singleton Math entry (pointing to the cpp.name!) One thing that is a little big about this change is a lot of kernels which previously didn't report as "math" now report as math. I picked a setting for these booleans that made sense to me, but I'm not sure if e.g. XLA will handle it 100% correctly. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]

Now that catchall declarations are reinterpreted as registrations to dispatch key Math, we can now simplify code generation logic by directly generating to Math, and bypasing logic for catchall. This also helps avoid bugs where we incorrectly classify some kernels as Math and others as not, even though they get registered in the same way. Bill of changes: - Give Math its own unique TORCH_LIBRARY_IMPL - Make it so NativeFunction.dispatch is always non-None. Simplify downstream conditionals accordingly - When parsing NativeFunction, fill in missing dispatch with a singleton Math entry (pointing to the cpp.name!) One thing that is a little big about this change is a lot of kernels which previously didn't report as "math" now report as math. I picked a setting for these booleans that made sense to me, but I'm not sure if e.g. XLA will handle it 100% correctly. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 2c9a1d0 Pull Request resolved: #46970

ezyang · 2020-10-28T05:25:33Z

codegen diff https://gist.github.com/ezyang/a6fb0178d5a44e5d760437902808566a

dr-ci · 2020-10-28T07:22:07Z

💊 CI failures summary and remediations

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

✅ None of the CI failures appear to be your fault 💚

1/1 broken upstream at merge base c886c7f on Oct 28 from 6:48am to 10:32am PDT (11 commits; 46b252b - 61ee024)

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_linux_backward_compatibility_check_test on Oct 28 from 6:48am to 10:32am PDT (11 commits; 46b252b - 61ee024)
- 🔁 rerun

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 5 times.

Now that catchall declarations are reinterpreted as registrations to dispatch key Math, we can now simplify code generation logic by directly generating to Math, and bypasing logic for catchall. This also helps avoid bugs where we incorrectly classify some kernels as Math and others as not, even though they get registered in the same way. Bill of changes: - Give Math its own unique TORCH_LIBRARY_IMPL - Make it so NativeFunction.dispatch is always non-None. Simplify downstream conditionals accordingly - When parsing NativeFunction, fill in missing dispatch with a singleton Math entry (pointing to the cpp.name!) One thing that is a little big about this change is a lot of kernels which previously didn't report as "math" now report as math. I picked a setting for these booleans that made sense to me, but I'm not sure if e.g. XLA will handle it 100% correctly. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D24592391](https://our.internmc.facebook.com/intern/diff/D24592391) [ghstack-poisoned]

bdhirsh · 2020-10-28T16:45:12Z

+    # them.  In native_functions.yaml, the dispatch entry is optional; in that
+    # case, that is equivalent to having written:
+    #
+    #   dispatch:


Is it worth deleting Math from all of the native_functions.yaml entries for consistency? Tentatively, I think making it an implementation detail that Math is the default dispatch entry makes more sense than explicitly offering two options for how people should register ops in native_functions.yaml.

I see only one Math entry, in a situation where it is mandatory:

- func: native_group_norm(Tensor input, Tensor? weight, Tensor? bias, int N, int C, int HxW, int group, float eps) -> (Tensor, Tensor, Tensor) use_c10_dispatcher: hacky_wrapper_for_legacy_signatures dispatch: CPU, CUDA: native_group_norm Math: math_group_norm

So we could enforce a canonical form where, if you only have a single Math entry, and its name matches the function schema name, then we can error saying that the Math entry is unnecessary. IDK if it's worth or not.

Ah yeah, I didn't notice that some ops require it (since they have other more specific dispatch keys). Probably not worth it then

ezyang · 2020-10-28T18:00:35Z

@ailzhang I would still love it if you can take a look, since this changes what meatdata we feed to xla.

ailzhang

Thanks!

ailzhang · 2020-10-28T18:43:30Z

        ('with_gil', False),
        ('deprecated', False),
-        ('has_math_kernel', f.dispatch is not None and 'Math' in f.dispatch),
+        ('has_math_kernel', 'Math' in f.dispatch),


This field is only used for the old RegistrationDeclarations.h. Should be good to remove now. ;)

facebook-github-bot · 2020-10-30T02:15:22Z

@ezyang merged this pull request in 54d8329.

Summary: Pull Request resolved: pytorch#46970 Now that catchall declarations are reinterpreted as registrations to dispatch key Math, we can now simplify code generation logic by directly generating to Math, and bypasing logic for catchall. This also helps avoid bugs where we incorrectly classify some kernels as Math and others as not, even though they get registered in the same way. Bill of changes: - Give Math its own unique TORCH_LIBRARY_IMPL - Make it so NativeFunction.dispatch is always non-None. Simplify downstream conditionals accordingly - When parsing NativeFunction, fill in missing dispatch with a singleton Math entry (pointing to the cpp.name!) One thing that is a little big about this change is a lot of kernels which previously didn't report as "math" now report as math. I picked a setting for these booleans that made sense to me, but I'm not sure if e.g. XLA will handle it 100% correctly. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Test Plan: Imported from OSS Reviewed By: albanD Differential Revision: D24592391 Pulled By: ezyang fbshipit-source-id: 2e3355f19f9525698864312418df08411f30a85d

This was referenced Oct 28, 2020

Delete Vulkan from code generator. #46938

Closed

Some miscellaneous cleanup in codegen #46940

Closed

ezyang requested review from ailzhang and bdhirsh October 28, 2020 05:23

ezyang mentioned this pull request Oct 28, 2020

Delete SchemaRegister.cpp, make flag operate on TypeDefault.cpp #46991

Closed

bdhirsh reviewed Oct 28, 2020

View reviewed changes

bdhirsh approved these changes Oct 28, 2020

View reviewed changes

This was referenced Oct 28, 2020

Delete TypeDefault call code generation logic in VariableType #47000

Closed

Delete TypeDefault.h and TypeDerived.h codegen entirely. #47002

Closed

ezyang mentioned this pull request Oct 28, 2020

Convert from higher order functions to classes in tools.codegen.gen #47008

Closed

ailzhang approved these changes Oct 28, 2020

View reviewed changes

ezyang mentioned this pull request Oct 28, 2020

ATen DerivedType is dead, long live ATen RegisterDispatchKey #47011

Closed

RockingJavaBean mentioned this pull request Oct 29, 2020

implement NumPy-like functionality column_stack, row_stack #46313

Closed

2 tasks

facebook-github-bot closed this in 54d8329 Oct 29, 2020

facebook-github-bot added the Merged label Oct 30, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desugar missing dispatch field into singleton Math entry#46970

Desugar missing dispatch field into singleton Math entry#46970
ezyang wants to merge 2 commits intogh/ezyang/857/basefrom
gh/ezyang/857/head

ezyang commented Oct 28, 2020 •

edited

Loading

Uh oh!

ezyang commented Oct 28, 2020

Uh oh!

dr-ci Bot commented Oct 28, 2020 •

edited

Loading

Uh oh!

bdhirsh Oct 28, 2020 •

edited

Loading

Uh oh!

ezyang Oct 28, 2020

Uh oh!

bdhirsh Oct 28, 2020

Uh oh!

ezyang commented Oct 28, 2020

Uh oh!

ailzhang left a comment

Uh oh!

ailzhang Oct 28, 2020

Uh oh!

facebook-github-bot commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 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!

ezyang commented Oct 28, 2020

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

🚧 1 fixed upstream failure:

Uh oh!

bdhirsh Oct 28, 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 Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

bdhirsh Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 28, 2020

Uh oh!

ailzhang left a comment

Choose a reason for hiding this comment

Uh oh!

ailzhang Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ezyang commented Oct 28, 2020 •

edited

Loading

dr-ci Bot commented Oct 28, 2020 •

edited

Loading

bdhirsh Oct 28, 2020 •

edited

Loading