Stream: git-wasmtime

Topic: wasmtime / PR #7948 cranelift: Remove redundant dominator...

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

jameysharp requested cfallin for a review on PR #7948.

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

jameysharp requested wasmtime-compiler-reviewers for a review on PR #7948.

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

jameysharp opened PR #7948 from jameysharp:domtree to bytecodealliance:main:

The egraph work introduced a helper for walking over the dominator tree in pre-order, but it turns out that helper already existed in the dominator_tree module itself, and the older version also supports constant-time dominance checks using the same trick that @cfallin suggested in #7891. So let's just use that version.

cc: @elliottt @lpereira

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

cfallin submitted PR review:

Woah,nice find! I had no idea we already had this (which I guess is why we wrote it again).

While I hesitate to violate the law that "any sufficiently advanced compiler contains three domtree implementations" (regalloc2 has one too!), I think we can do some spring cleaning here :-)

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

cfallin submitted PR review:

Woah,nice find! I had no idea we already had this (which I guess is why we wrote it again).

While I hesitate to violate the law that "any sufficiently advanced compiler contains three domtree implementations" (regalloc2 has one too!), I think we can do some spring cleaning here :-)

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

cfallin created PR review comment:

Can we update the name and comment here to match the type ("a preorder iterator over the domtree" or something similar)?

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

cfallin submitted PR review:

Woah, nice find! I had no idea we already had this (which I guess is why we wrote it again).

While I hesitate to violate the law that "any sufficiently advanced compiler contains three domtree implementations" (regalloc2 has one too!), I think we can do some spring cleaning here :-)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:28):

jameysharp updated PR #7948.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:31):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:31):

jameysharp created PR review comment:

When I looked again today at these modules to decide what to rename the fields to and what documentation to write, I discovered that the original DominatorTree isn't actually needed anywhere once we have a DominatorTreePreorder available. There was one debug_assert! using the original tree for dominance queries, but we can do those in O(1) time against the preorder instead. So I've renamed domtree_children just to domtree, replacing the other domtree field entirely.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:47):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:47):

cfallin created PR review comment:

Nice!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 20:56):

lpereira commented on PR #7948:

LGTM!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2024 at 22:29):

jameysharp merged PR #7948.

Last updated: Nov 22 2024 at 16:03 UTC