make canonicalize-nan.wast test work#9878
make canonicalize-nan.wast test work#9878alexcrichton merged 3 commits intobytecodealliance:mainfrom
Conversation
Lordworms
requested review from
cfallin and
fitzgen
and removed request for
a team
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "cranelift", "pulley"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
pulley/src/interp.rs
Outdated
| 0x7fc00000 | ||
| } else { | ||
| val.to_bits() | ||
| }; |
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this comment.
I am new to this project and didn't find it. Sorry for that.
Lordworms
force-pushed
the
canonicalize
branch
from
b350227 to
7fded6a
Compare
| "spec_testsuite/simd_f32x4_rounding.wast", | ||
| "spec_testsuite/simd_f64x2.wast", | ||
| "spec_testsuite/simd_f64x2_arith.wast", | ||
| "spec_testsuite/simd_f64x2_cmp.wast", | ||
| "spec_testsuite/simd_f64x2_pmin_pmax.wast", | ||
| "spec_testsuite/simd_f64x2_rounding.wast", |
There was a problem hiding this 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.
Lordworms
force-pushed
the
canonicalize
branch
from
0315a76 to
728fd17
Compare
crates/wast-util/src/lib.rs
Outdated
| "spec_testsuite/simd_f32x4_cmp.wast", | ||
| "spec_testsuite/simd_f32x4_pmin_pmax.wast", | ||
| "spec_testsuite/simd_f32x4_rounding.wast", | ||
| "spec_testsuite/simd_f64x2.wast", |
There was a problem hiding this comment.
Oh I think this test popped back in by accident during the rebase
3 participants
part of #9783