Repository navigation
Conversation
|
Can we maybe have a commit removing the old implementation and tests? It is not used anyway. Could also be a separate PR, but I think a separate commit should be fine as well: solidity/libsolutil/DominatorFinder.h Line 58 in 0944010 |
blishko
left a comment
blishko
left a comment
There was a problem hiding this comment.
I have some initial comments. Will have to look at the whole thing properly later.
I would have done it separately |
c5e697f to
f61197e
Compare
blishko
left a comment
blishko
left a comment
There was a problem hiding this comment.
Some more comments.
The property test looks reasonable on first reading.
I will try to understand the dominator algorithm.
| { | ||
| auto const numBlocks = static_cast<NodeID>(_exits.size()); | ||
|
|
||
| SSACFG cfg(EVMDialect::strictAssemblyForEVM(langutil::EVMVersion::current())); |
There was a problem hiding this comment.
Unrelated, but I am wondering if we could decouple the general graph structure representation from EVM specific properties.
There was a problem hiding this comment.
It absolutely can be decoupled but then we have to drag around the dialect. I put it into the graph so it's part of its definition. But maybe that wasn't the best idea :)
1cb9307 to
d0016c3
Compare
blishko
left a comment
blishko
left a comment
There was a problem hiding this comment.
I still don't fully understand the dominator tree construction, but the property test seems reasonable to me.
I have one more question, but otherwise, I think we can merge.
| // GTW06, Section 2.3 (b): compute immediate dominators | ||
| // | ||
| // For each node in preorder, walk from its DFS-tree parent up toward the root (via already-computed idoms) until | ||
| // reaching a node at or above its semidominator |
There was a problem hiding this comment.
| // reaching a node at or above its semidominator | |
| // reaching a node at or below its semidominator |
no?
There was a problem hiding this comment.
i meant above in the tree sense, ie, closer to the root. in the sense of index comparison you're right of course. I have reworded it so that it's not the ambiguous "above" or "below" anymore:
For each node in preorder, walk from its DFS-tree parent up toward the root (via already-computed idoms) until reaching a node whose preorder index is at most that of its semidominator, i.e., the semidominator itself or a DFS tree ancestor of it
7a864fa to
7672790
Compare
7672790 to
3c3d813
Compare
4 participants
Also adds dominator tree implementation for that purpose with its own small property test