Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cranelift: Use bump allocation in remove_constant_phis pass #4710

Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Aug 15, 2022

This makes compilation 2-6% faster for Sightglass's bz2 benchmark:

compilation :: cycles :: benchmarks/bz2/benchmark.wasm

  Δ = 7290648.36 ± 4245152.07 (confidence = 99%)

  bump.so is 1.02x to 1.06x faster than main.so!

  [166388177 183238542.98 214732518] bump.so
  [172836648 190529191.34 217514271] main.so

compilation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm

  No difference in performance.

  [182220055 225793551.12 277857575] bump.so
  [193212613 227784078.61 277175335] main.so

compilation :: cycles :: benchmarks/spidermonkey/benchmark.wasm

  No difference in performance.

  [3848442474 4295214144.37 4665127241] bump.so
  [3969505457 4262415290.10 4563869974] main.so

This makes compilation 2-6% faster for Sightglass's bz2 benchmark:

```
compilation :: cycles :: benchmarks/bz2/benchmark.wasm

  Δ = 7290648.36 ± 4245152.07 (confidence = 99%)

  bump.so is 1.02x to 1.06x faster than main.so!

  [166388177 183238542.98 214732518] bump.so
  [172836648 190529191.34 217514271] main.so

compilation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm

  No difference in performance.

  [182220055 225793551.12 277857575] bump.so
  [193212613 227784078.61 277175335] main.so

compilation :: cycles :: benchmarks/spidermonkey/benchmark.wasm

  No difference in performance.

  [3848442474 4295214144.37 4665127241] bump.so
  [3969505457 4262415290.10 4563869974] main.so
```
@fitzgen fitzgen requested a review from cfallin August 15, 2022 18:52
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

Some thoughts below for potential improvements while we're here, but feel free to defer if you'd prefer.

cranelift/codegen/src/remove_constant_phis.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/remove_constant_phis.rs Outdated Show resolved Hide resolved
I wasn't able to measure any perf difference here, but its nice to do anyways.
@fitzgen fitzgen enabled auto-merge (squash) August 15, 2022 20:40
@fitzgen fitzgen merged commit ae76880 into bytecodealliance:main Aug 15, 2022
@fitzgen fitzgen deleted the bump-alloc-in-remove-constant-phis branch August 15, 2022 22:37
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.

3 participants