backend: add multi-block support to RA - #6217
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6217 +/- ##
==========================================
+ Coverage 86.88% 86.89% +0.01%
==========================================
Files 430 430
Lines 64394 64419 +25
Branches 7385 7390 +5
==========================================
+ Hits 55948 55977 +29
+ Misses 6877 6875 -2
+ Partials 1569 1567 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8a093c0 to
1222784
Compare
|
|
||
| def allocate_registers(self, allocator: BlockAllocator) -> None: | ||
| """ | ||
| Allocates registers to the operands of this operation. |
There was a problem hiding this comment.
Can you please change the doc string to something like this?
| Allocates registers to the operands of this operation. | |
| Allocates registers to the operands of this operation. | |
| The successors must already be allocated, else raises PassFailedException. | |
| Allocates operands corresponding to block arguments to corresponding registers, then allocates rs1 and rs2. |
It would be nice to be defensive here to catch cases that aren't yet supported and have a nice error when we hit them.
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
There was a problem hiding this comment.
can you please add a test with a loop to see what happens there? The reason why we added the live in check was to check that values that are live-in to a loop were fully excluded from the loop body allocation, as they're live until the end of the loop iteration, and not until their last use in lexical order. I'm a little concerned about removing that check entirely, and it feels like having similar logic for cycles in blocks would be wise.
Samielakkad
left a comment
There was a problem hiding this comment.
The multi-block direction makes sense, but I think add_regalloc_stats changed behavior in a surprising way.
Before this PR, stats were computed once after the function block was allocated and one riscv.comment was inserted before the function. Now the stats block sits inside:
for block in PostOrderIterator(func.body.blocks[0]):
...
if add_regalloc_stats:
...
Rewriter.insert_op(..., InsertPoint.before(func))So a multi-block function will insert one stats comment per block, all before the same function. The stats are also computed over only block.walk() at that point, while the old comment described the whole function allocation.
If the intent is still function-level stats, this probably wants to move back outside the block loop and walk func.body after all blocks are allocated. If block-level stats are intended, the comment text should probably include the block name/order so repeated comments are interpretable.
This PR is a simple test to see if a PO walk would give us a baseline register allocator for multiple blocks.