core: preorder walk of blocks in nested operations #3367
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3367 +/- ##
==========================================
+ Coverage 91.27% 91.28% +0.01%
==========================================
Files 461 462 +1
Lines 57656 57745 +89
Branches 5571 5578 +7
==========================================
+ Hits 52623 52714 +91
+ Misses 3609 3608 -1
+ Partials 1424 1423 -1 ☔ View full report in Codecov by Sentry. |
…alkOrder::Preorder in MLIR
| @@ -967,6 +967,13 @@ def walk( | |||
| if region_first: | |||
There was a problem hiding this comment.
Perhaps rename the post_order.py file to traversals.py and move it there?
There was a problem hiding this comment.
I think it makes sense to keep this logic in ir.py
xdsl/ir/core.py
Outdated
| @@ -967,6 +967,13 @@ def walk( | |||
| if region_first: | |||
| yield self | |||
|
|
|||
| def walk_blocks_preorder(self) -> Iterator[Block]: | |||
| for region in self.regions: | |||
| for block in region.blocks: | |||
There was a problem hiding this comment.
This is not what I would have expected a preorder traversal of blocks to do. Does llvm simply iterate over them in the order they appear?
There was a problem hiding this comment.
Now that I think about it, it would probably be worth adding a walk method on Block that mirrors the API of the Operation walk, and to change this to forward to that one. I also prefer our walk_regions_first to Preorder as it's more declarative, maybe for blocks it could be walk_child_blocks_first? pre-post order doesn't make sense in the absence of left and right children
There was a problem hiding this comment.
I more meant that I would have expected a "traversal" to explore blocks via a depth first search of successors from each block, rather than just return each block in order
There was a problem hiding this comment.
Ah no that's not what I would expect TBH. @gabrielrodcanal, if you don't feel like implementing reversed iteration and post-order then I think it would still be worth adding walk on Block, and walk_blocks on Operation, and we can add the optional parameters later without API breaking.
|
Any updates on this? Will you have some time to iterate on it this week? |
|
Taking over this PR, hope that's ok @gabrielrodcanal |
I'm interested in having a look if you don't mind; I'll relinquish if it's too much for my time ATM |
Preorder walk of blocks in nested operations. This corresponds to the
behaviour of WalkOrder::Preorder in MLIR. The test in the PR was
developed in parallel with an analysis pass in MLIR to ensure equivalent
behaviour in xDSL. For reproducibility, the relevant method in MLIR is:
```
void PreorderBlocks::print(raw_ostream &os) const {
operation->walk<WalkOrder::PreOrder>([&](Block *block) {
os << "--- Block: \n";
block->print(os);
os << "\n";
});
}
```
---------
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Preorder walk of blocks in nested operations. This corresponds to the behaviour of WalkOrder::Preorder in MLIR. The test in the PR was developed in parallel with an analysis pass in MLIR to ensure equivalent behaviour in xDSL. For reproducibility, the relevant method in MLIR is: