Skip to content

[jit][script] Distinguish TupleLiteral from ListLiteral#10128

Closed
suo wants to merge 1 commit intomasterfrom
suo/tuple_literal
Closed

[jit][script] Distinguish TupleLiteral from ListLiteral#10128
suo wants to merge 1 commit intomasterfrom
suo/tuple_literal

Conversation

@suo
Copy link
Member

@suo suo commented Aug 1, 2018

Previously, the parser was emitting list literals for tuples, but the IR was representing list literals internally with TupleTypes.

For implementing most list operations, I think it will be helpful distinguish between lists (dynamic size, homogeneous types) and tuples (fixed arity, heterogeneous types)

This diff modifies the parser logic to emit tuple literals. This frees us to represent lists as ListType in the IR, while still properly mapping tuple literals to TupleTypes.

A following diff will actually switch over list literals to emit ListTypes.

@suo suo added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 1, 2018
@suo suo requested review from jamesr66a and zdevito August 1, 2018 17:32
@suo suo force-pushed the suo/tuple_literal branch from a38b2b9 to ba32f4f Compare August 1, 2018 17:34
Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
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.

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

@suo
Copy link
Member Author

suo commented Aug 1, 2018

Looks I'll need to rebase this on top of #10027 and fix up the ListLiteral construction there as well. I guess I'll just wait until it properly lands.

Copy link
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.

LGTM

Previously, the parser was emitting list literals for tuples, but we
were using TupleType as the internal representation, so it was okay.

If we intend to handle lists of dynamic size, we can't use TupleType,
since TupleTypes of different arity are different types.

This diff switches over list literals to use ListType as their internal
representation, and distinguishes tuple literals in the parser logic so
they can be properly mapped to TupleTypes of known arity.
@suo suo force-pushed the suo/tuple_literal branch from ba32f4f to 5df3117 Compare August 1, 2018 23:27
Copy link
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.

michaelsuo is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@suo suo deleted the suo/tuple_literal branch August 2, 2018 03:13
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Summary:
Previously, the parser was emitting list literals for tuples, but the IR was representing list literals internally with TupleTypes.

For implementing most list operations, I think it will be helpful distinguish between lists (dynamic size, homogeneous types) and tuples (fixed arity, heterogeneous types)

This diff modifies the parser logic to emit tuple literals. This frees us to represent lists as ListType in the IR, while still properly mapping tuple literals to TupleTypes.

A following diff will actually switch over list literals to emit ListTypes.
Pull Request resolved: pytorch#10128

Differential Revision: D9121305

Pulled By: michaelsuo

fbshipit-source-id: e0cad07ae8bac680f7f8113d10e5129d5a1a511d
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants