Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55): Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55):bjorn3 requested abrown for a review on PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55):bjorn3 requested wasmtime-compiler-reviewers for a review on PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55):bjorn3 opened PR #9511 from bjorn3:deny_implicit_sret_by_default to bytecodealliance:main:
It implements an incorrect ABI and may be removed in the future due to complexity reasons. By requiring to enable an option to use it, it becomes harder to accidentally hit the ABI issue and allows a more deprecation and eventual removal.
Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55):bjorn3 requested elliottt for a review on PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 16:55):bjorn3 requested wasmtime-core-reviewers for a review on PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 16:59):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
Can we add a paragraph at the end here recommending that the embedder develop their own multi-return convention as the most stable option? Something like:
Because of the above issues, and complexities of native ABI support for the concept in general, Cranelift's support for multiple return values may also be removed in the future (#9510). For the most robust solution, it is recommended to build a convention on top of Cranelift's primitives for passing multiple return values, for example by allocating a stackslot in the caller, passing it as an explicit argument, storing return values in the callee, and loading results in the caller.
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
s/violates the ABI of all targets/currently does not conform to platform ABIs/
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
can we link an issue number here?
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin submitted PR review:
Looks good, thanks -- I agree it's a good idea to wall this off a little bit while we work out what to do. A few comments on the description and error messages to offer some more tips but otherwise LGTM.
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
Can we link the issue here as well?
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
In these errors can we mention #9510 as well?
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:06):cfallin created PR review comment:
s/back compat/backward compatibility/
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:32):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 17:51):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 24 2024 at 18:27):cfallin commented on PR #9511:
@bjorn3 it looks like a few riscv64 tests need the new option as well. Let me know when green and I'm happy to take a final look and merge!
Wasmtime GitHub notifications bot (Oct 25 2024 at 07:22):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 25 2024 at 07:34):CI is green.
Wasmtime GitHub notifications bot (Oct 27 2024 at 00:45):cfallin commented on PR #9511:
A few more failures in this run when it tried the full set on the merge queue, it looks like. (Feel free to use
prtest:fullin a commit message to test this on the branch!)
Wasmtime GitHub notifications bot (Oct 27 2024 at 10:03):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 27 2024 at 10:16):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 27 2024 at 10:32):bjorn3 updated PR #9511.
Wasmtime GitHub notifications bot (Oct 27 2024 at 10:32):Fixed those tests.
Wasmtime GitHub notifications bot (Oct 27 2024 at 18:21):cfallin merged PR #9511.
Last updated: Nov 22 2024 at 17:03 UTC