Skip to content

Add tuple indexing support for constant integers#11492

Closed
eellison wants to merge 7 commits intopytorch:masterfrom
eellison:tuple_index_constants
Closed

Add tuple indexing support for constant integers#11492
eellison wants to merge 7 commits intopytorch:masterfrom
eellison:tuple_index_constants

Conversation

@eellison
Copy link
Copy Markdown
Contributor

@eellison eellison commented Sep 10, 2018

Add support indexing tuples with constant integers by creating a new prim::TupleIndex operator.

@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 10, 2018
apaszke
apaszke reviewed Sep 10, 2018
View reviewed changes
Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/register_prim_ops.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the tuple_index_constants branch from 48cbfdb to 81b4152 Compare September 11, 2018 01:44
@eellison
Copy link
Copy Markdown
Contributor Author

eellison commented Sep 11, 2018

@pytorchbot retest this please

apaszke
apaszke reviewed Sep 11, 2018
View reviewed changes
Comment thread test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/register_prim_ops.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke reviewed Sep 12, 2018
View reviewed changes
Comment thread test/test_jit.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

Comment thread torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

zdevito
zdevito reviewed Sep 12, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple issues with how the prim::TupleIndex op works, and how constants are discovered in the compiler.

Comment thread torch/csrc/jit/ir.h Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp 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

zdevito
zdevito reviewed Sep 12, 2018
View reviewed changes
Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the tuple_index_constants branch from 4d24f86 to 110b786 Compare September 13, 2018 15:50
facebook-github-bot
facebook-github-bot reviewed Sep 13, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@eellison eellison force-pushed the tuple_index_constants branch 2 times, most recently from c5f28dd to 8e10ca7 Compare September 14, 2018 01:26
zdevito
zdevito reviewed Sep 14, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments for the slicing logic.

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/constants.h 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

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the tuple_index_constants branch 2 times, most recently from 5e029af to 03b972b Compare September 14, 2018 22:08
@yf225
Copy link
Copy Markdown
Contributor

yf225 commented Sep 25, 2018

@eellison What would be the next step for this PR?

@apaszke apaszke mentioned this pull request Oct 5, 2018
Closed
@zasdfgbnm zasdfgbnm mentioned this pull request Oct 5, 2018
Closed
zdevito
zdevito reviewed Oct 8, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly ready to go, but I think there are two bugs. First remove tuples modifies the insert point on the graph. Second, I think there is a missing beg <= end check.

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/passes/lower_tuples.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

Comment thread torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the tuple_index_constants branch from 03b972b to e4f7296 Compare October 10, 2018 21:15
zdevito
zdevito approved these changes Oct 22, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you get a chance can you rebase this?

@eellison eellison force-pushed the tuple_index_constants branch from e4f7296 to b21e0e1 Compare October 23, 2018 18:55
@eellison
Copy link
Copy Markdown
Contributor Author

eellison commented Oct 23, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Oct 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

facebook-github-bot
facebook-github-bot reviewed Oct 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 24, 2018
@facebook-github-bot
Add tuple indexing support for constant integers (#11492)
d686c41 
Summary:
Add support indexing tuples with constant integers by creating a new prim::TupleIndex operator.
Pull Request resolved: pytorch/pytorch#11492

Differential Revision: D9811996

Pulled By: eellison

fbshipit-source-id: a458c2522b3c81476252d920e27a8d6c7b9a036b
@ezyang ezyang added the merged label Jun 26, 2019
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@facebook-github-bot
Add tuple indexing support for constant integers (pytorch#11492)
8e3e235 
Summary:
Add support indexing tuples with constant integers by creating a new prim::TupleIndex operator.
Pull Request resolved: pytorch#11492

Differential Revision: D9811996

Pulled By: eellison

fbshipit-source-id: a458c2522b3c81476252d920e27a8d6c7b9a036b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zdevito zdevito zdevito approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

+2 more reviewers

@apaszke apaszke apaszke left review comments

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@eellison @yf225 @zdevito @apaszke @facebook-github-bot @ezyang @pytorchbot