Stream: git-wasmtime

Topic: wasmtime / PR #7993 Winch: Use 16 bit numbers for registe...

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:35):

jeffcharles requested elliottt for a review on PR #7993.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:35):

jeffcharles requested wasmtime-compiler-reviewers for a review on PR #7993.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:35):

jeffcharles opened PR #7993 from jeffcharles:fix-400-params-test to bytecodealliance:main:

<!--
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
-->
Using 16 bits instead of 8 bits to track the register index allows us to support parameter lists that are longer than 400. It looks like v8 supports 1000 params.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:35):

jeffcharles requested wasmtime-default-reviewers for a review on PR #7993.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:43):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:43):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2024 at 22:43):

saulecabrera created PR review comment:

Could we also update the aarch64 abi given that we're here? It still uses u8.

Wasmtime GitHub notifications bot (Feb 23 2024 at 23:04):

jeffcharles submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2024 at 23:04):

jeffcharles created PR review comment:

I noticed the RegIndexEnv implementation in aarch64 returns Option<u8> with a value of None if the index is incremented past the limit field and the limit field defaults to 8. There is one place where we call set_limit with a value of 1. So as far as I can tell, if we had over 8 parameters, the increment function would no longer be called so, with more than 255 parameters, an overflow would not happen like was the case with the x86 implementation.

Does it still make sense to use a u16 instead of a u8?

Wasmtime GitHub notifications bot (Feb 23 2024 at 23:14):

jeffcharles updated PR #7993.

Wasmtime GitHub notifications bot (Feb 24 2024 at 01:54):

github-actions[bot] commented on PR #7993:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

• saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 26 2024 at 14:05):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 26 2024 at 14:05):

saulecabrera created PR review comment:

Yeah good point -- thinking a bit more about this, I think that the root of the issue is the lack of limits for each register class. Perhaps instead of widening the type to u16 we should add an upper-bound and make increment return None. The upper-bound could be either:

• A limit like in the case of aarch64, which could be derived from the calling convention.
• Using a checked add in increment (u8::MAX is good enough to represent the register arguments)

Wasmtime GitHub notifications bot (Feb 26 2024 at 14:37):

fitzgen commented on PR #7993:

Note that we follow the JS embedding limits in Wasmtime for maximum compatibility with Web engines: https://webassembly.github.io/spec/js-api/index.html#limits

Wasmtime GitHub notifications bot (Feb 28 2024 at 16:50):

jeffcharles updated PR #7993.

Last updated: Dec 23 2024 at 13:07 UTC