Stream: git-wasmtime

Topic: wasmtime / PR #8140 Migrate all existing cranelift-filete...

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

alexcrichton opened PR #8140 from alexcrichton:migrate-more-tests to bytecodealliance:main:

All existing tests are moved and updated so now they're reflecting what Wasmtime is generating. Yay!

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

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

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

alexcrichton requested elliottt for a review on PR #8140.

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

alexcrichton requested fitzgen for a review on PR #8140.

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

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

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

jameysharp commented on PR #8140:

I'm trying to diff between old tests and new ones to get a sense of what's really changed. For example,

git diff main:cranelift/filetests/filetests/isa/x64/wasm HEAD:tests/disas/load-store/x64/

It seems odd that on x64, load_store_dynamic_kind_i32_index_0_guard_no_spectre_i32_access_0_offset.wat previously used a conditional branch to a trap, but now uses a cmov of a null pointer and lets the memory access trap.

Perhaps the script needs to emit settings for enable_heap_access_spectre_mitigation=false?

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

alexcrichton updated PR #8140.

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

alexcrichton commented on PR #8140:

Oops, indeed! That's a difference between Cranelift's defaults and Wasmtime's defaults I forgot about with the script. Rebased with a fix now

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

jameysharp commented on PR #8140:

For the table-*-fixed-size.wat tests, you've changed them from "optimize" tests to "clif" tests. If you're going to change the type of these tests in this PR, I'd rather go the other direction, with full opt_level=speed optimization. On the other hand if this is meant to be a no-op-ish PR, then these should be test = "optimize with opt_level=none.

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

jameysharp commented on PR #8140:

As far as I can tell, this is the series of renames you did, which I needed to reproduce in order to diff the real changes:

mv cranelift/filetests/filetests/wasm/load-store/* tests/disas/load-store/
mv cranelift/filetests/filetests/isa/aarch64/wasm/* tests/disas/load-store/aarch64/
mv cranelift/filetests/filetests/isa/x64/wasm/* tests/disas/load-store/x64/
mv cranelift/filetests/filetests/isa/riscv64/wasm/* tests/disas/load-store/riscv64/
mv cranelift/filetests/filetests/isa/s390x/wasm/* tests/disas/load-store/s390x/
mv cranelift/filetests/filetests/wasm/*.wat tests/disas
for f in tests/disas/load-store/riscv64/zb?.wat; do mv $f tests/disas/riscv64-$(basename $f) -i; done

After that, git diff -R shows the actual test changes.

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

alexcrichton updated PR #8140.

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

alexcrichton updated PR #8140.

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

alexcrichton commented on PR #8140:

you've changed them from "optimize" tests to "clif" tests

Oops yes I crossed the wires reproducing what was there before, thanks for catching!

this is the series of renames you did

Sorry you had to reconstruct this! You gave be a better idea of how to organize this though, I've now inserted a commit which explicitly moves everything with no changes, which means that while the overall diff is still messy the per-commit view is now easier to see what changed in each test.

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

alexcrichton updated PR #8140.

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

jameysharp submitted PR review:

That helped a ton, thank you! Could you apply the same opt_level=0 change to the non-fixed-size table-get/set tests too? Otherwise I think these all match, if my quick read of the diff didn't miss anything.

Very excited to start writing more tests with this new infrastructure. Thank you!

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

alexcrichton updated PR #8140.

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

alexcrichton has enabled auto merge for PR #8140.

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

alexcrichton merged PR #8140.

Last updated: Nov 22 2024 at 17:03 UTC