Skip to content

[Parser] Start to parse instructions #4789

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 1 commit into from
Jul 11, 2022
Merged

[Parser] Start to parse instructions #4789

merged 1 commit into from
Jul 11, 2022

Conversation

tlively
Copy link
Member

@tlively tlively commented Jul 11, 2022

Update gen-s-parser.py to produce a second version of its parsing code that
works with the new wat parser. The new version automatically replaces the s
element argument in the existing parser with the ctx and in arguments used
by the new parser, so adding new instructions will not require any additional
work in gen-s-parser.py after this change.

Also add stub make*** functions to the new wat parser, with a few filled out,
namely makeNop, makeUnreachable, makeConst, and makeRefNull. Update the
global parser to parse global initializer instructions and update
wat-kitchen-sink.wast to demonstrate that the instructions are parsed correctly.

Adding new instruction classes will require adding a new make*** function to
wat-parser.cpp in additional to wasm-s-parser.{h,cpp} after this change, but
adding a trivial failing implementation is good enough for the time being, so I
don't expect this to appreciably increase our maintenance burden in the near
term.

The infrastructure for parsing folded instructions, instructions with operands,
and control flow instructions will be implemented in future PRs.

Update gen-s-parser.py to produce a second version of its parsing code that
works with the new wat parser. The new version automatically replaces the `s`
element argument in the existing parser with the `ctx` and `in` arguments used
by the new parser, so adding new instructions will not require any additional
work in gen-s-parser.py after this change.

Also add stub `make***` functions to the new wat parser, with a few filled out,
namely `makeNop`, `makeUnreachable`, `makeConst`, and `makeRefNull`. Update the
`global` parser to parse global initializer instructions and update
wat-kitchen-sink.wast to demonstrate that the instructions are parsed correctly.

Adding new instruction classes will require adding a new `make***` function to
wat-parser.cpp in additional to wasm-s-parser.{h,cpp} after this change, but
adding a trivial failing implementation is good enough for the time being, so I
don't expect this to appreciably increase our maintenance burden in the near
term.

The infrastructure for parsing folded instructions, instructions with operands,
and control flow instructions will be implemented in future PRs.
@tlively tlively requested review from aheejin and kripken July 11, 2022 05:09
@tlively
Copy link
Member Author

tlively commented Jul 11, 2022

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Result<typename Ctx::GlobalTypeT> globaltype(Ctx&, ParseInput&);

// Instructions
template<typename Ctx>
Copy link
Member

Choose a reason for hiding this comment

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

These declarations could be autogenerated using an include of wasm-delegations.def

Copy link
Member Author

@tlively tlively Jul 11, 2022

Choose a reason for hiding this comment

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

This would be tricky because some of them have extra arguments, such as the calls with their isReturn arguments, i31 and struct get operations with their signed_ argument, and various other instructions with operation arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Could we get the isReturn from the name, if we read it again? Or is it lost at that point?

Copy link
Member

Choose a reason for hiding this comment

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

(In the old parser that was possible, though not sure if we actually used that ability or not)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the names are lost, but we could have the generated code pass the names into the parsing functions as additional arguments. I'm not sure generating the declarations would really save us much work though, since I don't see a way to generate the definitions as well.

} else {
return Ok{};
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Does SFINAE or such let us write a template def more than once? if so we could emit a failing stub for all the instructions (using wasm-delegations.def) and then just add the working ones after that.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, you can't define the same template twice, unfortunately.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Makes sense, we should do the same as the old parser with the function signatures. A tiny bit more work for now when adding a new instruction but no big deal.

@tlively tlively merged commit 5aa2e18 into main Jul 11, 2022
@tlively tlively deleted the parse-insts branch July 11, 2022 22:14
aheejin added a commit that referenced this pull request Jul 12, 2022
aheejin added a commit that referenced this pull request Jul 12, 2022
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