Move fused RNN kernels into ATen #10305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

apaszke wants to merge 3 commits into pytorch:master from apaszke:fused_rnn_move

Contributor

apaszke commented Aug 7, 2018

As in the title. I also did a small refactor that let us loose almost 400 loc. This is a first step in moving the RNN code to C++.


          Move fused RNN kernels to ATen

4966cc1

apaszke requested review from colesbury, ezyang, gchanan, soumith and zdevito as code owners

August 7, 2018 15:08

Contributor Author

apaszke commented Aug 7, 2018

cc @csarofeen (author of the kernels)

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Contributor

ezyang commented Aug 7, 2018

Can we get a billing of changes?

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

Contributor Author

apaszke commented Aug 7, 2018 •

edited

Loading

Summary of changes:

Kernel changes

Gradients are now optional (can be nullptr) in the backward LSTM kernel. This lets us avoid allocating a zero-filled tensor in backward.
Changed slightly how the indexing works (ATen's TensorInfo doesn't match 100% with THC's). Biases should always be 1D, all other tensors are either 2D, or are flattened to their 1D views when they are contiguous

Dispatch code changes

We only instantiate 2 specializations for every kernel now (we created 4 previously)
Dispatch code relies on templates entirely now
Abstracted certain duplicated patterns into helper functions
Used more verbose names for variables

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view


          Remove fused kernels from THCUNN

b5c2ca3

ezyang reviewed

View reviewed changes

aten/src/ATen/native/cuda/RNN.cu Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

tools/autograd/gen_python_functions.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed

View reviewed changes

torch/nn/_functions/rnn.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Contributor

ezyang commented Aug 7, 2018

This all seems basically fine. I didn't approve it yet because I want docs on NEVER_SKIP, everything else is optional.

apaszke force-pushed the fused_rnn_move branch from 9f423f2 to 97c9812 Compare

August 7, 2018 16:06


          Build and review fixes

1a78db4

apaszke force-pushed the fused_rnn_move branch from 97c9812 to 1a78db4 Compare

August 7, 2018 16:07

ezyang approved these changes

View reviewed changes

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

apaszke has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot closed this in

be5fb8f

zdevito pushed a commit to zdevito/ATen that referenced this pull request


          Move fused RNN kernels into ATen (#10305)

c13d8ca

Summary:
As in the title. I also did a small refactor that let us loose almost 400 loc. This is a first step in moving the RNN code to C++.
Pull Request resolved: pytorch/pytorch#10305

Reviewed By: ezyang

Differential Revision: D9196227

Pulled By: apaszke

fbshipit-source-id: 54da905519aade29baa63ab1774a3ee1db5663ba

goodlux pushed a commit to goodlux/pytorch that referenced this pull request


          Move fused RNN kernels into ATen (pytorch#10305)

c8b4636

Summary:
As in the title. I also did a small refactor that let us loose almost 400 loc. This is a first step in moving the RNN code to C++.
Pull Request resolved: pytorch#10305

Reviewed By: ezyang

Differential Revision: D9196227

Pulled By: apaszke

fbshipit-source-id: 54da905519aade29baa63ab1774a3ee1db5663ba

mys007 mentioned this pull request

AttributeError: module 'torch.nn._functions.thnn' has no attribute 'rnnFusedPointwise' loicland/superpoint_graph#98

Closed

ezyang added open source merged labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ssnl ssnl left review comments

facebook-github-bot facebook-github-bot left review comments

ezyang ezyang approved these changes

colesbury Awaiting requested review from colesbury

gchanan Awaiting requested review from gchanan

soumith Awaiting requested review from soumith

zdevito Awaiting requested review from zdevito

Labels