core: preorder walk of blocks in nested operations

[gabrielrodcanal]

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";
  });
}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.11%. Comparing base (e6c7282) to head (15e5d31).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3367    +/-   ##
========================================
  Coverage   90.10%   90.11%            
========================================
  Files         449      450     +1     
  Lines       56599    56762   +163     
  Branches     5429     5456    +27     
========================================
+ Hits        51001    51150   +149     
- Misses       4164     4170     +6     
- Partials     1434     1442     +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superlopuh]

could you please change this test to work with only the test dialect, to avoid spurious dependencies?

[alexarice]

Perhaps rename the post_order.py file to traversals.py and move it there?

[alexarice]

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?

[superlopuh]

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

[alexarice]

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

[superlopuh]

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.

[superlopuh]

sorry to be annoying, but do you think you could change this from parsing the text to constructing it in Python? Something like this would make the test a little easier to read and understand, IMO. then we could just assert tuple(op.walk_blocks()) == (...)

[superlopuh]

It feels natural for it to have this name, WDYT? It would be worth testing also, probably in the same pytest

Suggested change

	def walk_child_blocks_first(self) -> Iterator[Block]:
	for op in self.ops:
	for region in op.regions:
	for block in region.blocks:
	yield block
	yield from block.walk_child_blocks_first()
	def walk(self) -> Iterator[Block]:
	for op in self.ops:
	for region in op.regions:
	for block in region.blocks:
	yield block
	yield from block.walk()

[superlopuh]

Any updates on this? Will you have some time to iterate on it this week?