Stream: git-wasmtime

Topic: wasmtime / PR #9845 Optimize constant offset loads/stores...

Wasmtime GitHub notifications bot (Dec 17 2024 at 22:40):

alexcrichton opened PR #9845 from alexcrichton:optimize-bounds-check to bytecodealliance:main:

This commit implements an optimization when lowering Wasm bytecode to CLIF to skip a bounds check when the offset in memory is statically known. This comes up in C/C++/Rust when static memory is addressed for example where LLVM emits an i32.const 0 as the base address and puts the address of the variable in the offset instruction field. This isn't necessary for 64-bit platforms but when explicit bounds-checks are required this can help to eliminate a constant-true bounds-check.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 17 2024 at 22:40):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9845.

Wasmtime GitHub notifications bot (Dec 17 2024 at 22:40):

alexcrichton requested fitzgen for a review on PR #9845.

Wasmtime GitHub notifications bot (Dec 17 2024 at 22:40):

alexcrichton requested wasmtime-core-reviewers for a review on PR #9845.

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode created PR review comment:

Could you add a few tests with interesting non-zero offsets as well?

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode created PR review comment:

        Imm64(zero_extended.signed())

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode created PR review comment:

        let zero_extended = (self.0.unsigned() << delta) >> delta;

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode created PR review comment:

Would it make sense to specify a max, like(memory 1 2), so that this can test for loads that are in-bounds on the minimum but not the maximum?

Wasmtime GitHub notifications bot (Dec 18 2024 at 01:09):

sunfishcode created PR review comment:

I'm confused; if it's a signed integer of the given width, why would we zero-extend it? It looks like this function ends with .signed(), but the one user of this function calls unsigned() on the result.

Wasmtime GitHub notifications bot (Dec 18 2024 at 15:25):

fitzgen created PR review comment:

so that this can test for loads that are in-bounds on the minimum but not the maximum?

I think this should be "in bounds of the maximum but not the minimum", but yeah +1

Wasmtime GitHub notifications bot (Dec 18 2024 at 15:25):

fitzgen created PR review comment:

Just to make this larger function a little bit less of a monolith, could we factor this snippet out into its own helper?

Wasmtime GitHub notifications bot (Dec 18 2024 at 15:25):

fitzgen submitted PR review:

LGTM modulo a nitpick below and Dan's comments as well.

Wasmtime GitHub notifications bot (Dec 18 2024 at 15:25):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Dec 18 2024 at 17:40):

alexcrichton created PR review comment:

Oops this is a copy/paste typo

Wasmtime GitHub notifications bot (Dec 18 2024 at 17:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 18 2024 at 17:48):

alexcrichton updated PR #9845.

Wasmtime GitHub notifications bot (Dec 18 2024 at 17:48):

alexcrichton has enabled auto merge for PR #9845.

Wasmtime GitHub notifications bot (Dec 18 2024 at 18:20):

alexcrichton merged PR #9845.

Last updated: Dec 23 2024 at 13:07 UTC