Stream: git-wasmtime

Topic: wasmtime / PR #8129 Add an `asm` test suite for Wasmtime


view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton opened PR #8129 from alexcrichton:test-wasmtime-codegen to bytecodealliance:main:

This commit adds a suite of tests at tests/asm/*.wat which is intended to replace the tests in cranelift/filetests/filetests/wasm. Tests are configured differently than before using Wasmtime CLI flags rather than a custom TOML-based configuration scheme. Otherwise though the same shape of tests is supported.

This commit migrates a small handful of tests as a showcase and bulk migration is left for a follow-up.

<!--
Please make sure you include the following information:

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
-->

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton requested fitzgen for a review on PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton requested wasmtime-default-reviewers for a review on PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:04):

alexcrichton requested jameysharp for a review on PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:05):

alexcrichton commented on PR #8129:

This was inspired by https://github.com/bytecodealliance/wasmtime/pull/8125 and the discussion there

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 03:32):

alexcrichton updated PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:02):

fitzgen submitted PR review:

Instead of tests/asm maybe we should call this tests/dis or tests/disas since there are CLIF "disassemblies" in there too?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:02):

fitzgen submitted PR review:

Instead of tests/asm maybe we should call this tests/dis or tests/disas since there are CLIF "disassemblies" in there too?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:02):

fitzgen created PR review comment:

Can we avoid manual parsing and do the TOML thing like the other tests? Can still use exact CLI flags by having the toml just contain a string of CLI flags for that particular setting.

But manual parsing is just annoying to extend down the line.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:21):

alexcrichton created PR review comment:

I was actually hoping we could skip TOML entirely since that felt heavyweight enough to justify with custom heaps/tables but with just a few booleans and such it feels sort of overkill. For example it feels overly verbose to do:

;;! flags = ["-Oopt-level=s", "-Ccranelift-has-avx"]

as opposed to:

;;! flags: -O opt-level=s -C cranelift-has-avx

I don't necessarily have a great feeling for how extensible we want this to be down the line though. I was sort of also taking inspiration from compiletest-based tests in rust-lang/rust, but I know the parsing there is sort of gnarly nowadays given all the directives it supports.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:24):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:24):

fitzgen created PR review comment:

I was thinking we would have a single string for all CLI flags rather than an array of strings of individual flags, which should make it a little less verbose:

;;! flags: "-O opt-level=s -C cranelift-has-avx"

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:24):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:24):

fitzgen created PR review comment:

I was sort of also taking inspiration from compiletest-based tests in rust-lang/rust, but I know the parsing there is sort of gnarly nowadays given all the directives it supports.

Yeah this is the kind of thing I am afraid of

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:25):

fitzgen edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:43):

alexcrichton updated PR #8129.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:44):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2024 at 14:44):

alexcrichton created PR review comment:

Ok, I've switched back to TOML


Last updated: Dec 23 2024 at 12:05 UTC