wasmtime / PR #9878 make canonicalize-nan.wast test work · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9878 make canonicalize-nan.wast test work

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms opened PR #9878 from Lordworms:canonicalize to bytecodealliance:main:

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms requested cfallin for a review on PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms requested wasmtime-compiler-reviewers for a review on PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms requested fitzgen for a review on PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms requested wasmtime-core-reviewers for a review on PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:22):

Lordworms requested wasmtime-default-reviewers for a review on PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 05:23):

Lordworms edited PR #9878:

part of https://github.com/bytecodealliance/wasmtime/issues/9783

Wasmtime GitHub notifications bot (Dec 20 2024 at 06:46):

github-actions[bot] commented on PR #9878:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "pulley"

Thus the following users have been cc'd because of the following labels:

fitzgen: pulley

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Dec 20 2024 at 07:31):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 07:31):

cfallin created PR review comment:

I'm surprised this is necessary: the intent of canonicalize-nan.wast is to test Cranelift's NaN-canonicalization pass, and this pass operates on the machine-independent IR (CLIF) so it should work for compilation to Pulley as well as any other target. The Pulley virtual machine shouldn't have to unconditionally canonicalize NaNs.

Could you check whether the pass is producing NaN-canonicalization sequences (which look like compares to test for NaN, then a select operator)?

I'm wondering whether this might have something to do with the has_vector_support boolean in Context::canonicalize_nans(); the comment there (that riscv64 is the only architecture without vector support) is no longer accurate now that we have Pulley in-tree; I'm not sure but perhaps that's related?

Wasmtime GitHub notifications bot (Dec 20 2024 at 16:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 16:24):

alexcrichton created PR review comment:

Oh I think this might be my fault, in the contribution docs for pulley it recommends running the test suite directly via wasmtime wast .../*.wast. When tests/misc_testsuite/simd/canonicalize-nan.wast is executed that's actually expected to fail (it even fails on native) and it requires an extra -W nan-canonicalization flag.

Chris is right though in that the manual canonicalization here shouldn't be necessary. @Lordworms you'll find though that -W nan-canonicalization isn't fully supported just yet:
Error: failed to run script file './tests/misc_testsuite/simd/canonicalize-nan.wast'

Caused by:
    0: failed directive on ./tests/misc_testsuite/simd/canonicalize-nan.wast:7:1
    1: Compilation error: Unsupported feature: should be implemented in ISLE: inst = `v10 = fcmp.f32x4 uno v7, v7`, type = `Some(types::I32X4)`
That being said this PR still gets the simd_f32x4_rounding.wast test passing, so with the canonicalization removed it should still be good to flag that test as passing

Wasmtime GitHub notifications bot (Dec 20 2024 at 16:25):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 16:25):

alexcrichton created PR review comment:

As a minor stylistic note, mind prefixing these instructions with v (e.g. vtrunc32x4) instead? I've been trying to use the v prefix for all simd/vector-related instructions so far

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:09):

Lordworms submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:09):

Lordworms created PR review comment:

yep

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:13):

Lordworms created PR review comment:

I am new to this project and didn't find it. Sorry for that.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:13):

Lordworms submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:24):

Lordworms updated PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:37):

Lordworms updated PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:39):

alexcrichton created PR review comment:

Oh this'll want to stay as these are test exemptions for a different compiler, Winch, unrelated to Pulley. Only the ones above are the ones affecting Pulley.

Also it looks like a merge conflict has creeped in due to this file, so if you want to rebase that should help resolve the conflict.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:43):

Lordworms updated PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:44):

Lordworms submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:44):

Lordworms created PR review comment:

resolved it.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:53):

alexcrichton created PR review comment:

Oh I think this test popped back in by accident during the rebase

Wasmtime GitHub notifications bot (Dec 20 2024 at 17:55):

Lordworms updated PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 18:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2024 at 18:02):

alexcrichton has enabled auto merge for PR #9878.

Wasmtime GitHub notifications bot (Dec 20 2024 at 18:23):

alexcrichton merged PR #9878.

Last updated: Dec 23 2024 at 12:05 UTC