loop analysis simplification #9613
Conversation
github-actions
bot
added
the
cranelift
There was a problem hiding this comment.
Thanks for the PR. A few thoughts below. I think there may be some aspect of subjective taste here; but I'm not a huge fan of very large iterator chain expressions, because they take a lot of "mental type-inference" to understand intermediate steps and how the chain works, and they're harder to modify and maintain over time. They're not a huge amount worse than explicit for-loops, but here they don't really seem better either; so I'm not sure I see the "simplification" aspect. Happy to hear your thoughts though.
| inst: pred_inst, .. | ||
| }| domtree.dominates(block, pred_inst, layout), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
This iterator chain is a little too convoluted to be easily readable, IMHO. One thing that might help is to pull out an is_loop_header helper?
It might also be nice to have a method on the domtree that returns an RPO iterator directly (impl Iterator... in return is fine). Then we could do something like:
for &block in domtree.cfg_rpo().filter(|&block| is_loop_header(cfg, domtree, block)) { ... }
| @@ -190,6 +190,19 @@ impl LoopAnalysis { | |||
| self.valid = false; | |||
| } | |||
|
|
|||
| // Determines if a block dominates any predecessor | |||
| // and thus is a loop header | |||
| fn check_loop_header( | |||
There was a problem hiding this comment.
Let's call this is_loop_header (check doesn't tell me anything about what it returns; usually implies an invariant check or something like that instead). And the doc-comment should end with a period.
There was a problem hiding this comment.
Was going to use is_loop_header but already in use by another function - check was best synonym but not ideal
github-actions
bot
added
the
cranelift:area:machinst
Some small simplifications I saw when looking through the loop analysis code, mostly regarding using inbuilt iterator method for greater simplicity