Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46): Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton requested saulecabrera for a review on PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton requested elliottt for a review on PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton opened PR #8243 from alexcrichton:migrate-winch-tests to bytecodealliance:main:
This commit builds on https://github.com/bytecodealliance/wasmtime/pull/8237 to migrate all Winch filetests to the top-level
tests/disas/test suite. All files are copied to atests/disas/winchfolder.This PR has a number of commit splitting out the various stages of this migration. The main changes are:
test = "winch"is now supported indisastests- Support for the Winch ABI in Cranelift is ungated (it no longer panics) to get
wasmtime compileworking to get the tests working- Winch tests now use ATT syntax instead of Intel syntax (the Cranelift default)
The next follow-up I'd like to do is to remove the filetest infrastructure for Winch, which will probably also remove the Winch executable itself. I'll do that as a follow-up though.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton requested fitzgen for a review on PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:46):alexcrichton requested wasmtime-core-reviewers for a review on PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 17:57):elliottt commented on PR #8243:
One change that we made to the winch disasm tests that I liked was to only print offsets after branch instructions (and always after a return). This made updates to filetests easier to read, as the diff was usually more precise. Would that be easy to add back in before this migration, or should we do that as a separate update?
Wasmtime GitHub notifications bot (Mar 26 2024 at 19:22):alexcrichton commented on PR #8243:
I saw that yeah from Winch but I ended up leaving it out in favor of printing all offsets. I was a bit confused as to why the address after a jump instruction was printed because that didn't account for fall-through-style jumps (e.g. it didn't reliably catch all jump targets). For after-ret I was also not sure that would handle the case of multiple
retinstructions in a function.I can add it back in though if desired!
Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):saulecabrera submitted PR review:
Looks good to me, thanks! I'd be in favor of considering adding back the file test formatting; even though not all cases are covered (as you've pointed out) the original motivation that Trevor and I discussed was to make any diffs easier to read.
Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):saulecabrera submitted PR review:
Looks good to me, thanks! I'd be in favor of considering adding back the file test formatting; even though not all cases are covered (as you've pointed out) the original motivation that Trevor and I discussed was to make any diffs easier to read.
Wasmtime GitHub notifications bot (Mar 26 2024 at 19:36):saulecabrera created PR review comment:
"The Winch calling convention is only implemented for x86_64 and aarch64"
Wasmtime GitHub notifications bot (Mar 26 2024 at 21:30):alexcrichton updated PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 21:31):alexcrichton updated PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 21:32):alexcrichton updated PR #8243.
Wasmtime GitHub notifications bot (Mar 26 2024 at 21:32):alexcrichton commented on PR #8243:
Ah ok that makes sense. I've updated with that added back in plus some comments
Wasmtime GitHub notifications bot (Mar 27 2024 at 00:03):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 27 2024 at 00:04):elliottt commented on PR #8243:
Thanks for changing the output back!
Wasmtime GitHub notifications bot (Mar 27 2024 at 14:28):alexcrichton updated PR #8243.
Wasmtime GitHub notifications bot (Mar 27 2024 at 14:28):alexcrichton has enabled auto merge for PR #8243.
Wasmtime GitHub notifications bot (Mar 27 2024 at 15:10):alexcrichton merged PR #8243.
Last updated: Nov 22 2024 at 16:03 UTC