Skip to content

Migrate all Winch filetests to tests/disas#8243

Merged
alexcrichton merged 13 commits intobytecodealliance:mainfrom
alexcrichton:migrate-winch-tests
Mar 27, 2024
Merged

Migrate all Winch filetests to tests/disas#8243
alexcrichton merged 13 commits intobytecodealliance:mainfrom
alexcrichton:migrate-winch-tests

Conversation

@alexcrichton
Copy link
Member

This commit builds on #8237 to migrate all Winch filetests to the top-level tests/disas/ test suite. All files are copied to a tests/disas/winch folder.

This PR has a number of commit splitting out the various stages of this migration. The main changes are:

  • test = "winch" is now supported in disas tests
  • Support for the Winch ABI in Cranelift is ungated (it no longer panics) to get wasmtime compile working 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.

@alexcrichton alexcrichton requested review from a team as code owners March 26, 2024 17:46
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team March 26, 2024 17:46
@elliottt
Copy link
Member

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?

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. labels Mar 26, 2024
@alexcrichton
Copy link
Member Author

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 ret instructions in a function.

I can add it back in though if desired!

Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexcrichton
Copy link
Member Author

Ah ok that makes sense. I've updated with that added back in plus some comments

@github-actions github-actions bot added the cranelift:area:x64 Issues related to x64 codegen label Mar 26, 2024
@elliottt
Copy link
Member

Thanks for changing the output back!

@alexcrichton alexcrichton enabled auto-merge March 27, 2024 14:28
@alexcrichton alexcrichton added this pull request to the merge queue Mar 27, 2024
Merged via the queue into bytecodealliance:main with commit 70b076d Mar 27, 2024
@alexcrichton alexcrichton deleted the migrate-winch-tests branch March 27, 2024 15:10
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Mar 27, 2024
With all Winch tests moved to `tests/disas` in bytecodealliance#8243 plus the support of
`wasmtime compile -C compiler=winch` this tool should in theory be
supplanted nowadays with other alternatives. This commit removes the
executable and the `winch-filetests` support.
github-merge-queue bot pushed a commit that referenced this pull request Mar 27, 2024
With all Winch tests moved to `tests/disas` in #8243 plus the support of
`wasmtime compile -C compiler=winch` this tool should in theory be
supplanted nowadays with other alternatives. This commit removes the
executable and the `winch-filetests` support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants