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: GVN uadd_overflow_trap #5520

Merged
merged 4 commits into from
Jan 5, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 5, 2023

No description provided.

This test was accidentally hitting a special case for bounds checks for when we
know that `offset + access_size < min_size` and can skip some steps. This
commit changes the `min_size` of the memory to zero so that we are forced to do
fully general bounds checks.
Although this improves our test sequence for duplicate loads with dynamic
memories, it unfortunately doesn't have any effect on sightglass benchmarks:

```
instantiation :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm

  No difference in performance.

  [34448 35607.23 37158] gvn_uadd_overflow_trap.so
  [34566 35734.05 36585] main.so

instantiation :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm

  No difference in performance.

  [44101 60449.62 92712] gvn_uadd_overflow_trap.so
  [44011 60436.37 92690] main.so

instantiation :: instructions-retired :: benchmarks/bz2/benchmark.wasm

  No difference in performance.

  [35595 36675.72 38153] gvn_uadd_overflow_trap.so
  [35440 36670.42 37993] main.so

compilation :: instructions-retired :: benchmarks/bz2/benchmark.wasm

  No difference in performance.

  [17370195 17405125.62 17471222] gvn_uadd_overflow_trap.so
  [17369324 17404859.43 17470725] main.so

execution :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm

  No difference in performance.

  [7055720520 7055886880.32 7056265930] gvn_uadd_overflow_trap.so
  [7055719554 7055843809.33 7056193289] main.so

compilation :: instructions-retired :: benchmarks/spidermonkey/benchmark.wasm

  No difference in performance.

  [683589861 683767276.00 684098366] gvn_uadd_overflow_trap.so
  [683590024 683767998.02 684097885] main.so

execution :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm

  No difference in performance.

  [46436883 46437135.10 46437823] gvn_uadd_overflow_trap.so
  [46436883 46437087.67 46437785] main.so

compilation :: instructions-retired :: benchmarks/pulldown-cmark/benchmark.wasm

  No difference in performance.

  [126522461 126565812.58 126647044] gvn_uadd_overflow_trap.so
  [126522176 126565757.75 126647522] main.so

execution :: instructions-retired :: benchmarks/bz2/benchmark.wasm

  No difference in performance.

  [653010531 653010533.03 653010544] gvn_uadd_overflow_trap.so
  [653010531 653010533.18 653010537] main.so
```
@fitzgen fitzgen requested a review from elliottt January 5, 2023 00:01
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Jan 5, 2023
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@fitzgen fitzgen merged commit f4a2d53 into bytecodealliance:main Jan 5, 2023
@fitzgen fitzgen deleted the gvn-uadd-overflow-trap branch January 5, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants