Closed
Bug 1505777
Opened 5 years ago
Closed 5 years ago
Update to Cranelift 0.23
Categories
(Core :: JavaScript: WebAssembly, task)
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(3 files, 4 obsolete files)
2.07 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
1.08 MB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
31.73 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
This updates Cranemonkey to Cranelift 0.23. Major changes: - The new fallthrough_return patch eliminates the need for manually removing return instructions. - The "deref" global value type changes to "load" and the offset is on the pointer rather than on the loaded value. - There's a new "iadd_imm" global value. - The heap base is now considered readonly, allowing redundant loads of the base pointer from TLS to be eliminated. - This version has the jump table feature, but it's disabled in this patch as it needs additional changes.
Attachment #9023623 -
Flags: review?(bbouvier)
Assignee | ||
Comment 1•5 years ago
|
||
This removes the workaround for Cranelift #491.
Assignee: nobody → sunfish
Attachment #9023624 -
Flags: review?(bbouvier)
Assignee | ||
Comment 2•5 years ago
|
||
This runs "mach vendor rust" and contains the actual Cranelift 0.23 update. I split it into a separate patch to make reviewing easier, but I can merge the patches to keep the patches self-contained.
Attachment #9023626 -
Flags: review?(bbouvier)
Assignee | ||
Updated•5 years ago
|
Attachment #9023624 -
Attachment is patch: true
Attachment #9023624 -
Attachment mime type: application/octet-stream → text/plain
Comment 3•5 years ago
|
||
Comment on attachment 9023623 [details] [diff] [review] update-cranelift Review of attachment 9023623 [details] [diff] [review]: ----------------------------------------------------------------- Many things look so much more natural now (loading from TlsBase + offset, in particular). LGTM, thanks. ::: js/src/wasm/cranelift/src/compile.rs @@ -122,5 @@ > - /// Remove the trailing return instruction from the current function to make room for a custom > - /// epilogue. > - /// > - /// Return the new function size in bytes, adjusted from size. > - fn remove_return_inst(&mut self, size: CodeOffset) -> CodeOffset { yay ::: js/src/wasm/cranelift/src/cpu.rs @@ +38,5 @@ > } > > impl From<settings::SetError> for BasicError { > fn from(err: settings::SetError) -> BasicError { > + BasicError::new(err.to_string()) I think we can do even better by just reusing the error trait that's used in Cranelift, so we don't even have the BasicError structure at all? (maybe as a follow up) ::: js/src/wasm/cranelift/src/wasm2clif.rs @@ +187,5 @@ > fn get_table(&mut self, func: &mut ir::Function, table: TableIndex) -> TableInfo { > // Allocate all tables up to the requested index. > + while self.tables.len() <= table.index() { > + let wtab = self.env.table(TableIndex::new(self.tables.len())); > + let vmctx = self.get_vmctx_gv(func); This can be hoisted above the loop, for clarity. @@ +428,5 @@ > > fn make_heap(&mut self, func: &mut ir::Function, index: MemoryIndex) -> ir::Heap { > + assert_eq!( > + index, > + MemoryIndex::new(0), This can be simplified as assert_eq!(index.index(), 0, ...); I think? @@ +540,5 @@ > let wsig = self.env.signature(sig_index); > > // TODO: When compiling asm.js, the table index in inferred from the signature index. > // Currently, WebAssembly doesn't support multiple tables. That may change. > + assert_eq!(table_index, TableIndex::new(0)); ditto
Attachment #9023623 -
Flags: review?(bbouvier) → review+
Updated•5 years ago
|
Attachment #9023624 -
Flags: review?(bbouvier) → review+
Comment 4•5 years ago
|
||
Comment on attachment 9023626 [details] [diff] [review] mach-vendor-rust Review of attachment 9023626 [details] [diff] [review]: ----------------------------------------------------------------- From a look at the Cargo.lock file, this is accurate. rs=me
Attachment #9023626 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 5•5 years ago
|
||
Review comments addressed.
Attachment #9023623 -
Attachment is obsolete: true
Assignee | ||
Comment 6•5 years ago
|
||
Attachment #9023624 -
Attachment is obsolete: true
Assignee | ||
Comment 7•5 years ago
|
||
Attachment #9023626 -
Attachment is obsolete: true
Assignee | ||
Comment 8•5 years ago
|
||
Comment on attachment 9023684 [details] [diff] [review] update-cranelift Review of attachment 9023684 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward r+.
Attachment #9023684 -
Flags: review+
Assignee | ||
Comment 9•5 years ago
|
||
Comment on attachment 9023685 [details] [diff] [review] call-indirect-globals Review of attachment 9023685 [details] [diff] [review]: ----------------------------------------------------------------- Carrying forward r+.
Attachment #9023685 -
Flags: review+
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 9023687 [details] [diff] [review] mach-vendor-rust Carrying forward r+.
Attachment #9023687 -
Attachment is patch: true
Attachment #9023687 -
Attachment mime type: application/octet-stream → text/plain
Attachment #9023687 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Comment 11•5 years ago
|
||
Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe8e6a201ed [Cranelift] Update to Cranelift 0.23. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/29649174d3e7 [Cranelift] Use the Cranelift global values for tables. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/027bf8dee1e5 Run mach-vendor-rust to update Cranelift. rs=bbouvier
Keywords: checkin-needed
Comment 12•5 years ago
|
||
Backed out 3 changesets (bug 1505777) for linux leakcheck failures on a CLOSED TREE Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8811545863bb0b3faf036d57a3b5480e7694e64 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=210673669&revision=f1e6ccbef48d843bff32d5b1c6d222041de98436 Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=210673715&repo=mozilla-inbound&lineNumber=1832 Log snippet: [task 2018-11-08T22:50:06.455Z] 22:50:06 INFO - TEST-INFO | Main app process: exit 0 [task 2018-11-08T22:50:06.456Z] 22:50:06 INFO - runtests.py | Application ran for: 0:00:17.673165 [task 2018-11-08T22:50:06.457Z] 22:50:06 INFO - zombiecheck | Reading PID log: /tmp/tmpGADrV9pidlog [task 2018-11-08T22:50:06.458Z] 22:50:06 INFO - ==> process 1057 launched child process 1078 [task 2018-11-08T22:50:06.458Z] 22:50:06 INFO - zombiecheck | Checking for orphan process with PID: 1078 [task 2018-11-08T22:50:06.458Z] 22:50:06 INFO - Stopping web server [task 2018-11-08T22:50:06.479Z] 22:50:06 INFO - Stopping web socket server [task 2018-11-08T22:50:06.500Z] 22:50:06 INFO - Stopping ssltunnel [task 2018-11-08T22:50:06.522Z] 22:50:06 INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes [task 2018-11-08T22:50:06.522Z] 22:50:06 INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes [task 2018-11-08T22:50:06.524Z] 22:50:06 INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 0 bytes [task 2018-11-08T22:50:06.524Z] 22:50:06 INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes [task 2018-11-08T22:50:06.524Z] 22:50:06 INFO - TEST-INFO | leakcheck | gpu process: leak threshold set at 0 bytes [task 2018-11-08T22:50:06.526Z] 22:50:06 INFO - [task 2018-11-08T22:50:06.527Z] 22:50:06 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1057 [task 2018-11-08T22:50:06.528Z] 22:50:06 INFO - [task 2018-11-08T22:50:06.529Z] 22:50:06 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->| [task 2018-11-08T22:50:06.531Z] 22:50:06 INFO - | | Per-Inst Leaked| Total Rem| [task 2018-11-08T22:50:06.532Z] 22:50:06 INFO - 0 |TOTAL | 27 4| 3977199 1| [task 2018-11-08T22:50:06.540Z] 22:50:06 INFO - 1595 |nsTArray_base | 4 4| 1797722 1| [task 2018-11-08T22:50:06.542Z] 22:50:06 INFO - [task 2018-11-08T22:50:06.542Z] 22:50:06 INFO - nsTraceRefcnt::DumpStatistics: 1733 entries [task 2018-11-08T22:50:06.543Z] 22:50:06 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsTArray_base [task 2018-11-08T22:50:06.544Z] 22:50:06 ERROR - TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray_base) [task 2018-11-08T22:50:06.544Z] 22:50:06 INFO - runtests.py | Running tests: end.
Flags: needinfo?(sunfish)
Comment 13•5 years ago
|
||
Pushed by rgurzau@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbdabbc30f33 [Cranelift] Update to Cranelift 0.23. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/400c83b3756b [Cranelift] Use the Cranelift global values for tables. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3aff6770a5 Run mach-vendor-rust to update Cranelift. rs=bbouvier
Comment 14•5 years ago
|
||
Seems that Bug 1501102 was the culprit for the leakchecks. This bug was relanded.
Flags: needinfo?(sunfish)
Comment 15•5 years ago
|
||
Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1450f97acc2 Backed out 3 changesets for linux build failures on a CLOSED TREE
Assignee | ||
Comment 16•5 years ago
|
||
This updates the patch with a fix for the tup build: new_types.rs is no longer being generated, so remove it from python/mozbuild/mozbuild/backend/cargo_build_defs.py. Carrying forward r+.
Attachment #9023684 -
Attachment is obsolete: true
Attachment #9023975 -
Flags: review+
Assignee | ||
Comment 17•5 years ago
|
||
Try run with the Btop build passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=848d53e24d25b53c6217624c57199b716381ff10
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Comment 18•5 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3fb1fce6a9 [Cranelift] Update to Cranelift 0.23. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb48d47b208 [Cranelift] Use the Cranelift global values for tables. r=bbouvier https://hg.mozilla.org/integration/mozilla-inbound/rev/d2013de59e87 Run mach-vendor-rust to update Cranelift. rs=bbouvier
Keywords: checkin-needed
Comment 19•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d3fb1fce6a9 https://hg.mozilla.org/mozilla-central/rev/4fb48d47b208 https://hg.mozilla.org/mozilla-central/rev/d2013de59e87
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•