Merged
Conversation
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.
Contributor
Author
|
Take a look at the Visitors are really not that different. Although I'd also like to replace them with something that isn't recursive. |
Contributor
|
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 |
Contributor
Author
|
Yeah for sure |
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.
Contributor
Author
|
@alexcrichton ok, I added some non-recursive traversals. See the latest commit. Still working on re-implementing graphviz support. |
alexcrichton
approved these changes
Aug 9, 2019
Contributor
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.