Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22): Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22):KGrewal1 requested cfallin for a review on PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22):KGrewal1 requested wasmtime-compiler-reviewers for a review on PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22):KGrewal1 opened PR #9613 from KGrewal1:loop_analysis to bytecodealliance:main:
Some small simplifications I saw when looking through the loop analysis code, mostly regarding using inbuilt iterator method for greater simplicity
Wasmtime GitHub notifications bot (Nov 15 2024 at 18:07):cfallin created PR review comment:
I'm not sure how I feel about this one either -- is this really more readable than the for-loop with an if-statement?
Wasmtime GitHub notifications bot (Nov 15 2024 at 18:07):cfallin created PR review 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_headerhelper?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)) { ... }
Wasmtime GitHub notifications bot (Nov 15 2024 at 18:07):cfallin submitted PR review:
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.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:00):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:00):KGrewal1 created PR review comment:
I think some of the complexity is in how I wrote the de structuring - changing it to
pred.inst, as opposed to doing it between the pipes. Also would a collect be most appropriate here - used extend to ensure exact same semantics as before with push, but thewhile let shoulddrainstackand so it is being extended from empty each time (in which case collect may make this more clear)
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:01):KGrewal1 updated PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):cfallin created PR review comment:
Let's call this
is_loop_header(checkdoesn'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.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):cfallin created PR review comment:
Full sentence is preferable: "A block is a loop header if it dominates any of its predecessors."
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):cfallin created PR review comment:
I think this would be a little clearer if we had a comment above the
stack.extend, something like "Push all predecessors of this header that this header dominates onto the stack."
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:32):KGrewal1 submitted PR review.
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:32):KGrewal1 created PR review comment:
Was going to use
is_loop_headerbut already in use by another function - check was best synonym but not ideal
Wasmtime GitHub notifications bot (Nov 15 2024 at 19:51):KGrewal1 updated PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 20:08):KGrewal1 updated PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 20:39):KGrewal1 updated PR #9613.
Wasmtime GitHub notifications bot (Nov 15 2024 at 21:22):KGrewal1 updated PR #9613.
Wasmtime GitHub notifications bot (Nov 19 2024 at 20:00):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 19 2024 at 20:22):cfallin merged PR #9613.
Last updated: Nov 22 2024 at 17:03 UTC