Closed Bug 1508559 Opened 7 years ago Closed 6 years ago

Add anyref-related instructions to Ion

Categories

(Core :: JavaScript: WebAssembly, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(5 files, 8 obsolete files)

Off the top of my head we have the following instructions that process anyref values in nontrivial ways, with perhaps more coming: - ref.null - ref.is_null (or ref.isnull, whatever it's called this week) - table.get - table.set - table.copy - table.grow Note that adding ref.null and ref.is_null will also require adding a little bit of support for the NullRef type, but most of the work for that has already been done elsewhere (bug 1505774).
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
WIP. Needs write barrier work for set_global, minimally. Also, the test cases in wasm/gc need to be split so that we can separate tests that use anyref from tests that use more general ref types and prototype GC features, since ion will not support those for the time being. And that probably requires some testingFunction to discriminate.
Attached file ion-instructions.bundle (obsolete) —
Julian, here's a mercurial bundle off of m-i 451119:7a8b861da55b that contains some base patches (for anyref boxing), test case separation (so that we can implement anyref for ion without implementing all of the struct machinery), enabling ion for reftypes, ion instructions for anyref et al, and some pending write barrier work. This is not landable -- the write barrier work has several hacks -- but it should be roughly correct, as far as it goes. I will update this patch every so often. To test this with baseline only, I usually do this from my build directory js/src/build-debug: $ ../jit-test/jit_test.py dist/bin/js -f --args="--no-wasm-ion" wasm/gc and there should be no failures (because baseline supports everything). To test this with ion only: $ ../jit-test/jit_test.py dist/bin/js -f --args="--no-wasm-baseline" wasm/gc This has a small number of failures. The current hypothesis is that these are all related to missing stack maps / gc scanning support in ion. One could test that hypothesis by re-applying the suppressGC patch; all the failures should go away. In particular, the barrier that is implemented in this patch set should be sufficient. I have not tried this yet.
Flags: needinfo?(jseward)
Attached file ion-instructions-v2.bundle (obsolete) —
With bugfixes, and after landing some patches. Running wasm jit-tests locally I get 12 errors, all in wasm/gc, all with --no-wasm-baseline or with default tiering (which should normally select ion).
Attached file ion-instructions-v3.bundle (obsolete) —

Rebased and with some more fixes. I get 12 failures in a debug jit-test run, 10 in a release run. These are all believed to be caused by missing tracing support, since we no longer have the suppressGC hack in place.

Fix a bug in Ion code generation for table.get: the return value from the instance method is not the anyref, but a raw pointer to a nonmoveable location that contains the anyref, or a null pointer signifying an error. In addition, the code that was there used AnyRef for the return value from the call but that needs to be MIRType::Pointer.

This patch is not completely clean (some formatting, some naming) but seems to be OK. With this applied: 6 errors in wasm/gc in a debug build and --no-wasm-baseline.

Attached patch bug1508559-fix-select-bug.patch (obsolete) — Splinter Review

Fix a bug in select on x64: we inadvertently got a 32-bit move. We don't necessarily want to land this patch as-is since it has a platform ifdef, but it'll be an OK placeholder.

Depends on: 1520478
Flags: needinfo?(jseward)
Attached file bug1508559-v6.tar (obsolete) —

Here's a revised, resequenced and rebased patchset, dependent on bug 1520478,
which introduces MIRType::RefOrNull for baseline. It incorporates the
tableset and select bug fixes, and further RefOrNull changes as required:

bug1508559-v6-1-Tgrow-fill-arg.diff
bug1508559-v6-2-ion-insns.diff
bug1508559-v6-3-factor-rabaldr.diff
bug1508559-v6-4-reorganize-baldr.diff
bug1508559-v6-5-enable-ion-gc.diff
bug1508559-v6-6-change-directives.diff

All prefixes of the first four (v6-1 to v6-4) build and test OK. They
introduce new functionality but don't enable it. v6-5 enables the
functionality and so causes a couple of failures due to missing GC support.
v6-6 enables tests and causes further failures at present.

Attachment #9029882 - Attachment is obsolete: true
Attachment #9032364 - Attachment is obsolete: true
Attachment #9032932 - Attachment is obsolete: true
Attachment #9036551 - Attachment is obsolete: true
Attachment #9037975 - Attachment is obsolete: true
Attachment #9038175 - Attachment is obsolete: true
Blocks: 1517924
Depends on: 1527259
Attached file bug1508559-v7.tar (obsolete) —

New patchset, based on m-c of 459338:1ba05f897328 (Fri Feb 15 13:50:07 2019
+0900)

First apply bug1527563-simplify-calls-3.diff, from that bug. Then the tarfile
contents, in order, bug1508559-v6-[1..6]*.diff.

Attachment #9042896 - Attachment is obsolete: true
Attached file bug1508559-v8.tar

Applies directly to m-c 459716:3d6a4067fd12. No other patches needed.

Attachment #9044115 - Attachment is obsolete: true

Factor common wasm write barrier functionality out of the baseline
compiler so that it can also be used by Ion.

We reorganize the nodes for handling global variable access so as to
allow for more shared code paths and to allow for write barriers on
pointer stores.

For pointer stores, the store is split in two: one part to compute the
address of the slot to be stored (direct or indirect) and one part to
perform the store. The address computation is different for indirect
and direct globals, but the ultimate loading and storing of the
global's value can then be handled by the same code in both cases.

For all loads, and for non-pointer stores, the LIR and code generation
paths have been merged into WasmLoadSlot/WasmStoreSlot nodes.

Attached file ion-reftypes.bundle

This bundle off of m-i 459842:7fc33c7516c3 has the two patches that are up for review on this bug, plus the rebased and somewhat reengineered reftypes-for-ion patch, plus a couple of patches to enable reftypes for ion and trigger more test cases.

(It obsoletes the tar file that is on this bug, but I'll not hide that yet.)

Blocks: 1528983
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/20450dceb946 Factor Rabaldr's write barrier code. r=jseward
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e70b536542 fix nu build bustage. r=bustage on CLOSED TREE
Keywords: leave-open

Add support to WasmIonCompile for the instructions in the reftypes
proposal: ref.null, ref.is_null, table.get, table.set, table.grow,
table.size.

Also add support for the ref.eq instruction from the gc proposal.

Also update the test suite so that we will not ion-compile test cases
that use gc features that are not landed here.

Note that this patch does not change the compiler-selection behavior:
If --wasm-gc is enabled then only the baseline compiler will be used;
if --wasm-gc is not enabled then no compiler will recognize these
opcodes. Enabling Ion for reftypes content is the subject of
subsequent work.

Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd22c49c63c Reorganize Baldr globals for write barriers. r=jseward

Julian, re the ion patch, notice the introduction of MWasmLoadRef, which loads a RefOrNull via a raw pointer. This parallels in many ways MWasmStoreRef, already introduced for the global variables patch, but the global variables code does not use MWasmLoadRef because that's not been necessary. But if it simplifies anything for the work you're doing on the stack maps there is no reason we could not change that.

I'm going to move the patch that enables ion for reftypes content to another bug, possibly a new one.

Keywords: leave-open

Backed out changeset 386a2afe9189 (Bug 1508559) for failing raptor tests in /home/cltbld/tasks/task_1550758926/build/raptor.json CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&classifiedState=unclassified&selectedJob=229641860&revision=386a2afe918980e2cd0762290ad04112a63bd3b5

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229641860&repo=mozilla-inbound&lineNumber=582

14:39:53 INFO - raptor-control-server received webext_status: __raptor_shutdownBrowser
14:39:53 INFO - raptor-control-server shutting down browser (pid: 3604)
14:39:53 INFO - 127.0.0.1 - - [21/Feb/2019 06:39:53] "POST / HTTP/1.1" 200 -
14:39:53 INFO - raptor-control-server received webext_status: Removed tab 3
14:39:53 INFO - PID 3604 | [Parent 3604, Gecko_IOThread] WARNING: pipe error (132): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 357
14:40:09 INFO - raptor-main removing webext /home/cltbld/tasks/task_1550758926/build/tests/raptor/webext/raptor
14:40:09 INFO - results-handler summarizing raptor test results
14:40:09 INFO - raptor-output error: no raptor test results found for raptor-wasm-godot-firefox
14:40:09 INFO - raptor-output error: no summarized raptor results found for raptor-wasm-godot-firefox
14:40:09 INFO - raptor-control-server shutting down control server
14:40:10 INFO - raptor-main finished
14:40:10 INFO - raptor-main TEST-UNEXPECTED-FAIL: no raptor test results were found for raptor-wasm-godot-firefox
14:40:10 ERROR - Return code: 1
14:40:10 WARNING - setting return code to 1
14:40:10 CRITICAL - PERFHERDER_DATA was seen 0 times, expected 1.
14:40:10 INFO - copying raptor results to upload dir:
14:40:10 INFO - /home/cltbld/tasks/task_1550758926/build/blobber_upload_dir/perfherder-data.json
14:40:10 INFO - copying raptor results from /home/cltbld/tasks/task_1550758926/build/raptor.json to /home/cltbld/tasks/task_1550758926/build/blobber_upload_dir/perfherder-data.json
14:40:10 CRITICAL - Error copying results /home/cltbld/tasks/task_1550758926/build/raptor.json to upload dir /home/cltbld/tasks/task_1550758926/build/blobber_upload_dir/perfherder-data.json
14:40:10 INFO - [Errno 2] No such file or directory: u'/home/cltbld/tasks/task_1550758926/build/raptor.json'
14:40:10 INFO - Running post-action listener: _package_coverage_data
14:40:10 INFO - Running post-action listener: _resource_record_post_action
14:40:10 INFO - Running post-action listener: process_java_coverage_data
14:40:10 INFO - Running post-action listener: stop_device
14:40:10 INFO - [mozharness: 2019-02-21 14:40:10.096866Z] Finished run-tests step (success)
14:40:10 INFO - Running post-run listener: _resource_record_post_run
14:40:10 INFO - Total resource usage - Wall time: 183s; CPU: 3.0%; Read bytes: 97591296; Write bytes: 606150656; Read time: 768; Write time: 42840
14:40:10 INFO - TinderboxPrint: CPU usage<br/>3.2%
14:40:10 INFO - TinderboxPrint: I/O read bytes / time<br/>97,591,296 / 768
14:40:10 INFO - TinderboxPrint: I/O write bytes / time<br/>606,150,656 / 42,840
14:40:10 INFO - TinderboxPrint: CPU idle<br/>1,409.6 (96.7%)
14:40:10 INFO - TinderboxPrint: CPU user<br/>38.1 (2.6%)
14:40:10 INFO - TinderboxPrint: Swap in / out<br/>0 / 0
14:40:10 INFO - install - Wall time: 14s; CPU: 13.0%; Read bytes: 0; Write bytes: 2146304; Read time: 0; Write time: 16
14:40:10 INFO - run-tests - Wall time: 170s; CPU: 2.0%; Read bytes: 94773248; Write bytes: 602587136; Read time: 768; Write time: 42784
14:40:10 WARNING - returning nonzero exit status 1
[taskcluster 2019-02-21T14:40:10.188Z] Exit Code: 1
[taskcluster 2019-02-21T14:40:10.188Z] User Time: 52.728s
[taskcluster 2019-02-21T14:40:10.188Z] Kernel Time: 7.044s
[taskcluster 2019-02-21T14:40:10.188Z] Wall Time: 3m39.253793816s
[taskcluster 2019-02-21T14:40:10.188Z] Result: FAILED
[taskcluster 2019-02-21T14:40:10.188Z] === Task Finished ===
[taskcluster 2019-02-21T14:40:10.188Z] Task Duration: 3m39.254002624s
[taskcluster 2019-02-21T14:40:10.555Z] Uploading artifact public/logs/localconfig.json from file logs/localconfig.json with content encoding "gzip", mime type "application/json" and expiry 2020-02-21T13:13:55.653Z
[taskcluster 2019-02-21T14:40:11.286Z] Uploading artifact public/test_info/resource-usage.json from file build/blobber_upload_dir/resource-usage.json with content encoding "gzip", mime type "application/json" and expiry 2020-02-21T13:13:55.653Z
[taskcluster:error] exit status 1

Flags: needinfo?(lhansen)
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe6ece04420 Backed out changeset 386a2afe9189 for failing raptor tests in /home/cltbld/tasks/task_1550758926/build/raptor.json CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

This was backed out for failure, presumably related to this msg earlier in the log:

JavaScript error: http://localhost:55780/wasm-godot/godot.js line 1 >
   WebAssembly.instantiate, line 334514: RuntimeError: index out of bounds
Status: RESOLVED → REOPENED
Flags: needinfo?(lhansen)
Resolution: FIXED → ---

I can repro on a plain release build on the try server (no webrender or other special sauce), but not with a release build built locally. https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230302571&repo=try&lineNumber=569

The error message references a completely unlikely line number:

JavaScript error: http://localhost:45230/wasm-godot/godot.js line 1 >
   WebAssembly.instantiate, line 1199265: RuntimeError: index out of bounds

Even if that's a wasm instruction offset it's very different from the one in the error message in comment 25.

Haha, reproduces when asking specifically for Ion:

./mach raptor-test -t raptor-wasm-godot-ion

All is well with the world.

Error comes from a trap in compiled code, not from any of the changed table/memory initialization code.

Incorrect platform support for wasmSelect that affected only Ion - I simplified a conditional move too much.

Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 1531020
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: