Closed Bug 1666939 Opened 4 years ago Closed 4 years ago

v128 cannot be used in untyped select instruction

Categories

(Core :: JavaScript: WebAssembly, defect, P3)

Product:
Core
Component:
JavaScript: WebAssembly
Version:
Firefox 83
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Tracking Flags:
Tracking Status
firefox83 --- fixed

People

(Reporter: nabeel, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1666939 - Add support for v128 in typed select instructions. r?lth
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.42 Safari/537.36

Steps to reproduce:

Build wasm from C++ using emscripten 2.0.0

Actual results:

Firefox Nightly reported an error during wasm validation:

wasm validation error: invalid types for old-style 'select'

Expected results:

Firefox Nightly should still be supporting old-style 'select'

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Javascript: WebAssembly
Product: Firefox → Core

(In reply to nabeel from comment #0)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.42 Safari/537.36

Steps to reproduce:

Build wasm from C++ using emscripten 2.0.0

Actual results:

Firefox Nightly reported an error during wasm validation:

wasm validation error: invalid types for old-style 'select'

Expected results:

Firefox Nightly should still be supporting old-style 'select'

Could you attach the files (html, js, wasm) required to reproduce this issue?

Flags: needinfo?(nabeel)

Unfortunately, I am not able to share the initial files where I experienced this. I did try to write a small example, but when I use a simple switch expression it does not trigger this error.

Flags: needinfo?(nabeel)

This is likely an issue with emscripten generating invalid wasm code. The 'old-style' select instruction is only valid when operating on [i32, i64, f32, f64] input types [1]. This error indicates one of the input types is not one of those types. A new encoding of the select instruction was added to handle more types, and emscripten should be using it.

I'd file an issue on the emscripten repo with repro steps to see if they can figure it out. If it's not an issue with emscripten, then I'll need some way to reproduce the issue in order to fix it.

[1] https://webassembly.github.io/reference-types/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-parametric-mathsf-select-t-ast

Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID

Do number types include v128?

I see that SpiderMonkey complains about this:

(module
(memory 1 1)
(func $v128.const.i32x4 (result v128)
(select
(v128.const i32x4 1 2 3 4)
(v128.const i32x4 1 2 3 4)
(i32.const 1)
)
)
)

with

failed to asynchronously prepare wasm: CompileError: wasm validation error: at offset 68: invalid types for old-style 'select'

I don't see a flag to enable SIMD in the shell (there is only a flag to disable it). V8, with the flag for SIMD, accepts that wasm.

(In reply to alonzakai from comment #5)

Do number types include v128?

I see that SpiderMonkey complains about this:

(module
(memory 1 1)
(func $v128.const.i32x4 (result v128)
(select
(v128.const i32x4 1 2 3 4)
(v128.const i32x4 1 2 3 4)
(i32.const 1)
)
)
)

with

failed to asynchronously prepare wasm: CompileError: wasm validation error: at offset 68: invalid types for old-style 'select'

I don't see a flag to enable SIMD in the shell (there is only a flag to disable it). V8, with the flag for SIMD, accepts that wasm.

Ah, that's a good point; a fun little interaction between the different proposals. The reference-types proposal introduces the 'number type' category and defines it as i32,i64,f32,f64 and that's how we've implemented it. The simd proposal is not based off the reference-types proposal, and so it's unclear what we should do here.

There's no reason we can't allow v128 in the untyped select instruction, I wouldn't expect it to ever have a subtyping relation that would motivate excluding it. But I think it's worth bringing up in the SIMD repo just to make sure there are no strong opinions against it.

Alon, was that snippet taken from generated code from llvm/emscripten/binaryen? I'm curious if any toolchains are currently generating code like this.

Flags: needinfo?(alonzakai)

That's a hand-written code sample by me. But the Binaryen optimizer can definitely optimize various patterns into a select (of any type, including v128).

I opened https://github.com/WebAssembly/simd/issues/363 on the spec repo.

Flags: needinfo?(alonzakai)
Summary: wasm validation error: old-style select → v128 cannot be used in untyped select instruction

Reopening until we know what we're doing here.

Blocks: wasm-simd
Status: RESOLVED → REOPENED
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: INVALID → ---
Severity: -- → S3
Status: REOPENED → NEW
Priority: -- → P3

I think it makes sense to make v128 a number type. In either case, we don't actually support v128 even with the typed select instruction. So there's some work to do here.

Assignee: nobody → rhunt
  • Reclassify v128 as a number type, introducing that category in our code base.
  • Add support to baseline and ion for v128 select.
  • Split off and extend test for select instruction.
    • Add tests for typed select with reftypes
    • Add tests for untyped select with v128
Attachment #9177912 - Attachment description: Bug 1666939 - Add support for v128 in typed and untyped select instructions. r?lth → Bug 1666939 - Add support for v128 in typed select instructions. r?lth
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/4065a1a701bb
Add support for v128 in typed select instructions. r=lth

Backed out changeset 4065a1a701bb (bug 1666939) for select.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=a6c0c3427a2f54bc8f28de833574b1328ed50f23&searchStr=build&tochange=548312dae23095e9d7d2887b67fcdabe2d96ca15&selectedTaskRun=KcyUfD_0RzWMe5vKL8dJIg.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/548312dae23095e9d7d2887b67fcdabe2d96ca15

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316940949&repo=autoland&lineNumber=45207

...
[task 2020-09-28T21:05:33.930Z] TEST-PASS | js/src/jit-test/tests/wasm/simd/js-api.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.3 s]
[task 2020-09-28T21:05:33.942Z] -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:33.942Z] Stack:
[task 2020-09-28T21:05:33.942Z]   @-e:1:6
[task 2020-09-28T21:05:33.942Z] Exit code: 3
[task 2020-09-28T21:05:33.942Z] FAIL - wasm/simd/select.js
[task 2020-09-28T21:05:33.942Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/simd/select.js | -e:1:6 ReferenceError: wasmSimdSupported is not defined (code 3, args "") [0.2 s]
[task 2020-09-28T21:05:33.942Z] INFO exit-status     : 3
[task 2020-09-28T21:05:33.942Z] INFO timed-out       : False
[task 2020-09-28T21:05:33.942Z] INFO stderr         2> -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:33.942Z] INFO stderr         2> Stack:
[task 2020-09-28T21:05:33.942Z] INFO stderr         2> @-e:1:6
[task 2020-09-28T21:05:33.971Z] TEST-PASS | js/src/jit-test/tests/wasm/simd/js-api.js | Success (code 0, args "--wasm-compiler=ion") [0.2 s]
[task 2020-09-28T21:05:33.971Z] TEST-PASS | js/src/jit-test/tests/wasm/simd/js-api.js | Success (code 0, args "--wasm-compiler=baseline") [0.2 s]
[task 2020-09-28T21:05:33.994Z] -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:33.994Z] Stack:
[task 2020-09-28T21:05:33.994Z]   @-e:1:6
[task 2020-09-28T21:05:33.994Z] Exit code: 3
[task 2020-09-28T21:05:33.994Z] FAIL - wasm/simd/select.js
[task 2020-09-28T21:05:33.994Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/simd/select.js | -e:1:6 ReferenceError: wasmSimdSupported is not defined (code 3, args "--no-blinterp --no-baseline --no-ion --more-compartments") [0.2 s]
[task 2020-09-28T21:05:33.994Z] INFO exit-status     : 3
[task 2020-09-28T21:05:33.995Z] INFO timed-out       : False
[task 2020-09-28T21:05:33.995Z] INFO stderr         2> -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:33.995Z] INFO stderr         2> Stack:
[task 2020-09-28T21:05:33.995Z] INFO stderr         2> @-e:1:6
[task 2020-09-28T21:05:34.009Z] -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:34.009Z] Stack:
[task 2020-09-28T21:05:34.009Z]   @-e:1:6
[task 2020-09-28T21:05:34.009Z] Exit code: 3
[task 2020-09-28T21:05:34.009Z] FAIL - wasm/simd/select.js
[task 2020-09-28T21:05:34.009Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/simd/select.js | -e:1:6 ReferenceError: wasmSimdSupported is not defined (code 3, args "--blinterp-eager") [0.2 s]
[task 2020-09-28T21:05:34.009Z] INFO exit-status     : 3
[task 2020-09-28T21:05:34.009Z] INFO timed-out       : False
[task 2020-09-28T21:05:34.009Z] INFO stderr         2> -e:1:6 ReferenceError: wasmSimdSupported is not defined
[task 2020-09-28T21:05:34.009Z] INFO stderr         2> Stack:
[task 2020-09-28T21:05:34.009Z] INFO stderr         2> @-e:1:6
[task 2020-09-28T21:05:34.026Z] -e:1:6 ReferenceError: wasmSimdSupported is not defined
...
Flags: needinfo?(rhunt)

Ah, landed two patches at the same time that conflicted with each other.

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/167a8edf5dee
Add support for v128 in typed select instructions. r=lth

https://hg.mozilla.org/mozilla-central/rev/167a8edf5dee

Status: NEW → RESOLVED
Closed: 4 years ago → 4 years ago
status-firefox83: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: