Wasmtime GitHub notifications bot (Nov 02 2023 at 17:45): Wasmtime GitHub notifications bot (Nov 02 2023 at 17:45):saulecabrera opened PR #7462 from saulecabrera:winch-add-spec-tests-for-locals to bytecodealliance:main:
This change is a follow up to https://github.com/bytecodealliance/wasmtime/pull/7443; after it landed I realized that Winch doesn't include spec tests for local.get and loca.set.
Those tests uncovered a bug on the handling of the constant pool: given Winch's singlepass nature, there's very little room know all the constants ahead of time and to register them all at once at emission time; instead they are emitted when they are needed by an instruction. Even though Cranelift's machinery is capable of deuplicated constants in the pool,
register_constantassumes and checks that each constat should only be pushed once. In Winch's case, since we emit as we go, we need to carefully check if the constant is one was not emitted before, and if that's the case, register it. Else we break the invariant that each constant should only be registered once.<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 02 2023 at 17:45):saulecabrera requested fitzgen for a review on PR #7462.
Wasmtime GitHub notifications bot (Nov 02 2023 at 17:45):saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7462.
Wasmtime GitHub notifications bot (Nov 02 2023 at 17:45):saulecabrera requested wasmtime-core-reviewers for a review on PR #7462.
Wasmtime GitHub notifications bot (Nov 02 2023 at 17:46):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Nov 02 2023 at 17:46):saulecabrera created PR review comment:
I decided to add this function here, but if there's a reason why this shouldn't be done, I could find a different mechanism guarantee the invariant that I'm after.
Wasmtime GitHub notifications bot (Nov 02 2023 at 23:17):saulecabrera edited PR review comment.
Wasmtime GitHub notifications bot (Nov 03 2023 at 00:27):saulecabrera edited PR #7462:
This change is a follow up to https://github.com/bytecodealliance/wasmtime/pull/7443; after it landed I realized that Winch doesn't include spec tests for local.get and local.set.
Those tests uncovered a bug on the handling of the constant pool: given Winch's singlepass nature, there's very little room know all the constants ahead of time and to register them all at once at emission time; instead they are emitted when they are needed by an instruction. Even though Cranelift's machinery is capable of deuplicated constants in the pool,
register_constantassumes and checks that each constat should only be pushed once. In Winch's case, since we emit as we go, we need to carefully check if the constant is one was not emitted before, and if that's the case, register it. Else we break the invariant that each constant should only be registered once.<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 03 2023 at 16:21):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 03 2023 at 16:21):fitzgen created PR review comment:
Seems like a reasonable method to me!
Wasmtime GitHub notifications bot (Nov 03 2023 at 16:22):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Nov 03 2023 at 17:28):saulecabrera merged PR #7462.
Last updated: Dec 23 2024 at 12:05 UTC