Stream: git-wasmtime

Topic: wasmtime / PR #5072 egraphs: a few miscellaneous compile-...

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:12):

cfallin opened PR #5072 from egraph-misc-opts to main:

These optimizations together are worth about a 2% compile-time reduction, as measured on one more with spidermonkey.wasm as an input, using hyperfine on wasmtime compile.

The changes included are:

• Some better pre-allocation (blockparams and side-effects concatenated list vecs);
• Avoiding the indirection of storing list-of-types for every Pure and Inst node, when almost all nodes produce only a single result; instead, store arity and single type if it exists, and allow result projection nodes to fill in types otherwise;
• Pack the MemoryState enum into one u32 (this together with the above removal of the type slice allows Node to shrink from 48 bytes to 32 bytes);
• always-inline an accessor (entry on CtxHash) that wasn't (always(inline) appears to be load-bearing, rather than just inline);
• Split the update-analysis path into two hotpaths, one for the union case and one for the new-node case (and the former can avoid recomputing for the contained node when replacing a node with node-and-child eclass entry).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:12):

cfallin requested elliottt for a review on PR #5072.

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:12):

cfallin requested fitzgen for a review on PR #5072.

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:14):

cfallin updated PR #5072 from egraph-misc-opts to main.

Wasmtime GitHub notifications bot (Oct 18 2022 at 22:14):

cfallin edited PR #5072 from egraph-misc-opts to main:

These optimizations together are worth about a 2% compile-time reduction, as measured on one core with spidermonkey.wasm as an input, using hyperfine on wasmtime compile.

The changes included are:

• Some better pre-allocation (blockparams and side-effects concatenated list vecs);
• Avoiding the indirection of storing list-of-types for every Pure and Inst node, when almost all nodes produce only a single result; instead, store arity and single type if it exists, and allow result projection nodes to fill in types otherwise;
• Pack the MemoryState enum into one u32 (this together with the above removal of the type slice allows Node to shrink from 48 bytes to 32 bytes);
• always-inline an accessor (entry on CtxHash) that wasn't (always(inline) appears to be load-bearing, rather than just inline);
• Split the update-analysis path into two hotpaths, one for the union case and one for the new-node case (and the former can avoid recomputing for the contained node when replacing a node with node-and-child eclass entry).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen created PR review comment:

                        arity: {
                            debug_assert!(results.len() <= (u16::MAX as usize));
                            results.len() as u16
                        },

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen created PR review comment:

Ditto

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen created PR review comment:

Can't just be #[inline]? Needs to be inlined in non-optimized builds too?

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:33):

fitzgen created PR review comment:

Ditto

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:47):

cfallin created PR review comment:

I'd prefer not to affect non-optimized builds, but for whatever reason the inline(always) seemed to be necessary to get inlining to happen in optimized builds, unfortunately...

(I'd really love to gain more insight into the heuristics at play here!)

Wasmtime GitHub notifications bot (Oct 18 2022 at 23:47):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 00:20):

cfallin updated PR #5072 from egraph-misc-opts to main.

Wasmtime GitHub notifications bot (Oct 19 2022 at 00:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 00:20):

cfallin created PR review comment:

Fixed! Actually used u16::try_from(...).expect(...) here but result is the same. And, factored out above if/else chain to use below as well.

Wasmtime GitHub notifications bot (Oct 19 2022 at 00:24):

cfallin updated PR #5072 from egraph-misc-opts to main.

Wasmtime GitHub notifications bot (Oct 19 2022 at 00:38):

cfallin has enabled auto merge for PR #5072.

Wasmtime GitHub notifications bot (Oct 19 2022 at 01:02):

cfallin has disabled auto merge for PR #5072.

Wasmtime GitHub notifications bot (Oct 19 2022 at 01:10):

cfallin updated PR #5072 from egraph-misc-opts to main.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:05):

cfallin merged PR #5072.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:14):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:14):

abrown created PR review comment:

Shouldn't this be self.0 & 3 > 0?

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:24):

cfallin created PR review comment:

It's an unpack method just for the tag=1 case so I think this is correct?

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:44):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:44):

abrown created PR review comment:

Ah, ok... It seemed like it could have been for all three :+1:

Last updated: Dec 23 2024 at 12:05 UTC