Add anyref-related instructions to Ion

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(5 attachments, 8 obsolete attachments)

100.00 KB, application/x-tar
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
12.40 KB, application/octet-stream
Details
47 bytes, text/x-phabricator-request
Details | Review
(Assignee)

Description

5 months ago
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)

Updated

5 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 months ago
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.
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1508556
(Assignee)

Comment 3

4 months ago
Posted 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)
(Assignee)

Comment 4

4 months ago
Posted 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).
(Assignee)

Comment 5

3 months ago
Posted 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.

(Assignee)

Comment 6

3 months ago

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.

(Assignee)

Comment 7

3 months ago

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)
Posted 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
Posted 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
Posted file bug1508559-v8.tar

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

Attachment #9044115 - Attachment is obsolete: true
(Assignee)

Comment 11

2 months ago

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

(Assignee)

Comment 12

2 months ago

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.

(Assignee)

Comment 13

2 months ago
Posted 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

Comment 14

2 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20450dceb946
Factor Rabaldr's write barrier code. r=jseward

Comment 15

2 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e70b536542
fix nu build bustage. r=bustage on CLOSED TREE
(Assignee)

Updated

2 months ago
Keywords: leave-open
(Assignee)

Comment 16

2 months ago

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.

Comment 18

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

Comment 19

2 months ago

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.

Comment 20

2 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/386a2afe9189
Ion instructions for reftypes. r=jseward
(Assignee)

Comment 21

2 months ago

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)

Comment 23

2 months ago
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

Comment 24

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
(Assignee)

Comment 25

2 months ago

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 → ---
(Assignee)

Comment 26

2 months ago

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.

(Assignee)

Comment 27

2 months ago

Haha, reproduces when asking specifically for Ion:

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

All is well with the world.

(Assignee)

Comment 28

2 months ago

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

(Assignee)

Comment 29

2 months ago

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

Comment 30

2 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96a0237880cf
Ion instructions for reftypes. r=jseward

Comment 31

2 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED

Updated

2 months ago
Depends on: 1531020
You need to log in before you can comment on or make changes to this bug.