Skip to content
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

List IR #114

Merged
merged 5 commits into from Aug 9, 2019
Merged

List IR #114

merged 5 commits into from Aug 9, 2019

Conversation

@fitzgen
Copy link
Member

fitzgen commented Aug 1, 2019

Still need to re-add graphviz dot support, so not quite ready to land yet. But the most important bits are already here, so I figured you might want to take a look now, @alexcrichton.

fitzgen added 3 commits Jul 30, 2019
This is going to need to be completely overhauled for the new IR, so it is
easier to just remove it outright and then add it back once we have the new IR
in place.
We had plans to build a find/replace style API with this, but that never
materialized, and nothing is taking advantage of this API, so let's just remove
it.
Instead of an AST, we have lists of instructions. This is more similar to raw
Wasm itself, and should make supporting multi-value even easier. There is still
nesting, in that each `block ... end`, `loop ... end`, and `if ... else ... end`
recursively contain new instruction sequences, rather than having their bodies
inlined into the current instruction sequence. I suspect this will make it
easier to slice, dice, and splice functions.
@fitzgen fitzgen requested a review from alexcrichton Aug 1, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Aug 1, 2019

Take a look at the build-wasm-from-scratch.rs example to get an idea of how builders have changed: https://github.com/fitzgen/walrus/blob/list-ir/examples/build-wasm-from-scratch.rs#L52-L97

Visitors are really not that different. Although I'd also like to replace them with something that isn't recursive.

@alexcrichton

This comment has been minimized.

Copy link
Collaborator

alexcrichton commented Aug 2, 2019

This looks great to me, thanks for taking this on @fitzgen! I think we'll probably want to flesh out the non-recursive visitation before this lands though to ensure we've got a working strategy for it.

Also would you be up for prototyping a patch to wasm-bindgen to use this walrus to double-check that it still works for wasm-bindgen?

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Aug 2, 2019

Yeah for sure

@fitzgen fitzgen force-pushed the fitzgen:list-ir branch from b3db05f to f82b764 Aug 8, 2019
This commit changes the way IR traversal is done, notably:

* Visitors are no longer recursive, and should never recursively call
  `self.visit_foo()` from inside `self.visit_bar()`. Not in the default provided
  trait methods and not in any user-written overrides of those trait methods.

* The `Visit` trait is no longer exported in the public API. Calling
  `my_instruction.visit(visitor)` will call *all* of the `visit_foo_id` methods
  of the visitor that are relevant for that instruction *but not recursively
  through nested instruction sequences*. For example, calling `visit` on a
  `Block` will call `visitor.visit_instr_seq_id`, but will not recursively visit
  the referenced `InstrSeq`.

* There are now *traversal functions* which take a visitor, a `LocalFunction`,
  and a start `InstrSeqId`, and then perform some kind of traversal over the
  function's IR from the given start sequence. These traversal functions
  are *not* recursive, and are implemented with explicit work lists and while
  loops. This avoids blowing the stack on deeply nested Wasm inputs. Although we
  can still OOM, we leave fixing that to future PRs. Right now there are only
  two traversals, because that is all we've needed so far: an in-order DFS for
  immutable visitors (needs to be in-order so we can encode instructions in the
  right order) and a pre-order DFS for mutable visitors (pre-order is the
  easiest traversal to implement iteratively). We can add more traversals as we
  need them.
@fitzgen fitzgen force-pushed the fitzgen:list-ir branch from f82b764 to f7cb704 Aug 8, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Aug 8, 2019

@alexcrichton ok, I added some non-recursive traversals. See the latest commit. Still working on re-implementing graphviz support.

Copy link
Collaborator

alexcrichton left a comment

Looks reasonable to me!

This is new and improved dot support. Instead of being specific to a single
function, this allows encoding the whole module as a dot file, including all
functions.
@fitzgen fitzgen force-pushed the fitzgen:list-ir branch from 9acb21a to 587d087 Aug 9, 2019
@fitzgen fitzgen merged commit 42d3b61 into rustwasm:master Aug 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fitzgen fitzgen deleted the fitzgen:list-ir branch Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.