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.
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
prototype walker #626
prototype walker #626
Changes from 6 commits
bb76660
6b7615c
27503f4
27f2595
fcc78d4
9973b72
4134536
e4cc9eb
9f2265c
c91a025
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pass the Hugr(View) in here and remove
self.hugr
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the callbacks need access to the hugr. In the current design the hugr is fixed for a given Walker, so the callbacks can capture the hugr. Passing the hugr here would mean that the Walker could be invoked on many hugrs, so the callbacks couldn't capture it. The hugr would need to be threaded through the callbacks. This can be done but I think it's less ergonomic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you actually put the WorkItem struct in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the definition? that would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Post DFS order" is a reverse topological sort? i.e. users of a value produced, before the node produces a value?
validate_children_dag
(validate.rs) - where we use a Topo. That definitely breaks for CFGs, but for DFGs appears to do in one pass what you're having to do two passes for here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree a better comment is needed. Post DFS is reverse topological sort, but we push them to a stack so they are visited in reverse Post DFS order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concerned that cfg nodes appear to be just skipped here. IIUC, guppy (for example) produces Hugrs where each function's body is a CFG (perhaps containing only a single basic block). Obviously "topological sort order" may not be viable for a CFG, but dfs postorder is....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One might want to handle Module nodes too, so you really can start this off for any Hugr; although that is maybe less pressing, as there is no chance of finding a Module nested within a DFG (whereas you might find CFGs/DFGs arbitrarily nested).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CFG and Module nodes work fine, they are caught be the second traversal. There is a test on Modules.
We do a (reverse) PostDFS traversal, which will visit nodes in topological order where this makes sense (when the sibling graph has an Input node), then a second traversal that catches anything else i.e. BasicBlocks or FuncDefns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see got it. I hadn't lined up the indenting correctly / realized the closing
}
. In that case this all looks good, but would be good to mention in the commentsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just take "So we do a second unordered traversal aftewards", put on a new line, and add "this covers non-dataflow sibling graphs too"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet there is some way with
enum_dispatch
but I'll leave that to someone Rustier than me.... @aborgna-q ? ;)A clone is no big deal, I'm happy with that