Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Oct 30, 2025

No description provided.

@kripken kripken requested a review from tlively October 30, 2025 17:21
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but this is getting pretty complicated due to the various ways we avoid or handle letting the parent map get out of date. It would be nice to think of an alternative approach if possible.

@kripken
Copy link
Member Author

kripken commented Oct 31, 2025

Where else do we avoid issues with the Parent map? I see one around "that local.set would not be reflected in the parent" but that seems like a separate issue, in that this issue could be fixed by recomputing Parents in each iteration, but that one can't - it is in the middle of an iteration. To fully fix both we'd need to constantly update Parents as we go, constantly, which would be more burdensome I think.

@tlively
Copy link
Member

tlively commented Nov 3, 2025

Maybe that's the only explicit place, but IIRC the order in which we optimize the children is also carefully chosen to avoid potential problems. In general there are just a lot of similar branches here with non-obvious edge cases to handle. I don't feel confident in my ability to reason about the correctness of new code here without relying entirely on the fuzzer to find problems.

Not the end of the world, but would be nice to simplify if we come up with a good idea.

@kripken
Copy link
Member Author

kripken commented Nov 3, 2025

Hmm, could be, yeah. We could just not rely on Parents at all, and use ExpressionStackWalker (which maintains a stack as we traverse). More overhead but could be worth it. However, reading the code, we don't just look at parents as we walk, but at random points in the graph (we can ask about the parent of a local.get that is related to us, somewhere entirely different in the function), so it might not be easy.

Another option could be to recompute Parents after each update that could affect those connections, but it might make us quadratic time...

A utility to update parents in lockstep with changes, like TypeUpdater, might work, but is not simple either.

@kripken
Copy link
Member Author

kripken commented Nov 3, 2025

Landing for now, but we can keep thinking about this.

@kripken kripken merged commit 7e782f6 into WebAssembly:main Nov 3, 2025
16 checks passed
@kripken kripken deleted the h2l.later branch November 3, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants