Stream: git-wasmtime

Topic: wasmtime / PR #9613 loop analysis simplification


view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22):

KGrewal1 requested cfallin for a review on PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 14:22):

KGrewal1 requested wasmtime-compiler-reviewers for a review on PR #9613.

view this post on Zulip 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

view this post on Zulip 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?

view this post on Zulip 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_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)) { ... }

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:00):

KGrewal1 submitted PR review.

view this post on Zulip 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 the while let should drain stack and so it is being extended from empty each time (in which case collect may make this more clear)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:01):

KGrewal1 updated PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:30):

cfallin created PR review 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.

view this post on Zulip 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."

view this post on Zulip 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."

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:32):

KGrewal1 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:32):

KGrewal1 created PR review comment:

Was going to use is_loop_header but already in use by another function - check was best synonym but not ideal

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 19:51):

KGrewal1 updated PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 20:08):

KGrewal1 updated PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 20:39):

KGrewal1 updated PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 21:22):

KGrewal1 updated PR #9613.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 20:00):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 20:22):

cfallin merged PR #9613.


Last updated: Dec 23 2024 at 12:05 UTC