Skip to content

Update Pop text format to handle tuples #3116

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.

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

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Sep 11, 2020

Previously Pops were printed as ({type}.pop), and if the popped type
was a tuple, something like ((i32, i64).pop) would get
printed. However, the parser didn't support pops of anything besides
single basic types.

This PR changes the text format to be (pop *) and adds support
for parsing pops of tuples of basic types. The text format change is
designed to make parsing simpler. This change is necessary for writing
Poppy IR tests that contain break or return instructions that consume
multiple values (see #3059).

Previously Pops were printed as ({type}.pop), and if the popped type
was a tuple, something like ((i32, i64).pop) would get
printed. However, the parser didn't support pops of anything besides
single basic types.

This PR changes the text format to be (pop <type>*) and adds support
for parsing pops of tuples of basic types. The text format change is
designed to make parsing simpler. This change is necessary for writing
Poppy IR tests that contain break or return instructions that consume
multiple values (see WebAssembly#3059).
@tlively tlively requested a review from kripken September 11, 2020 04:16
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

LGTM.

This change is necessary for writing Poppy IR tests that contain break or return instructions that consume multiple values (see #3059).

Why is this? I checked #3059 but am not sure. Can you elaborate?

@tlively
Copy link
Member Author

tlively commented Sep 11, 2020

This change is necessary for writing Poppy IR tests that contain break or return instructions that consume multiple values (see #3059).

Why is this? I checked #3059 but am not sure. Can you elaborate?

Return and Break instruction classes only take a single child, so that child has to have a tuple type if the function or break target expects multiple values. In Poppy IR, that means the child has to be a tuple-typed pop, so it is useful to be able to write those in the text format. I'll elaborate in the commit message as well.

@tlively tlively merged commit 8ec8a0b into WebAssembly:master Sep 11, 2020
@tlively tlively deleted the pop-syntax branch September 11, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants