1608771 - Implement Ion->wasm inlined calls for BigInt <-> i64 interconversion

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

When Ion code calls wasm, we can take an especially fast path by inlining the call in MCallOptimize, given that the types are right. For the case where the passed type is known to be BigInt, the BigInt -> int64 range folding can be done with a simple MIR node and then the call can simply load the known-int64 value as part of the call.

Comment 1

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 1, BigInt<>I64 conversion for inlined calls

This is part 1 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part implements Ion MIR and LIR instructions that are needed for conversion between BigInts and I64.

Comment 2

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 2, BigInt<>I64 conversion for inlined calls

This is part 2 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part adds an I64 return value case for LIonToWasmCall, which is needed in general because an I64
result may require multiple defs for the instruction.

Comment 3

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 3, BigInt<>I64 conversion for inlined calls

This is part 3 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part adds changes needed in code generation and Wasm stubs for supporting the I64 arguments for
inlined Ion to Wasm calls.

Comment 4

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 4, BigInt<>I64 conversion for inlined calls

This is part 4 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part adds the support for I64/BigInt arguments for inlined calls in the MCallOptimize.cpp part of
the IonBuilder. With this commit, the I64 arguments will work on 64-bit platforms, except where arguments
are required to be spilled to the stack due to the ABI (this case is more complicated to support).

On 32-bit platforms, this commit disables the BigInt/I64 support for the inlined call as it does not work
without further changes.

Comment 5

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 5, BigInt<>I64 conversion for inlined calls

This is part 5 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part adds additional Wasm BigInt tests that are aimed specifically to test limits of the inlined calls and
to test more conversion cases that should be covered (e.g., to test ToBigInt instruction cases).

Comment 6

[Asumu Takikawa]

Attachment: Bug 1608771 - Part 6, BigInt<>I64 conversion for inlined calls

This is part 6 of implementing the Wasm BigInt<>I64 conversion proposal for inlined Ion to Wasm calls.

This part adds support for I64/BigInt arguments for inlined calls on 32-bit platforms. This requires additional
code because I64s are split into two parts on 32-bit platforms, which adds additional work to every part
from the original MIR instruction generation down to Wasm stub code.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3588d7de8ea8
Part 1, BigInt<>I64 conversion for inlined calls r=wingo,lth

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3588d7de8ea8

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91bc171d94e3
Part 2, BigInt<>I64 conversion for inlined calls r=lth,wingo

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/91bc171d94e3

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02be9fd2d743
Part 3, BigInt<>I64 conversion for inlined calls r=lth,wingo

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/02be9fd2d743

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b6dbbb27b74
Part 4, BigInt<>I64 conversion for inlined calls r=lth,wingo
https://hg.mozilla.org/integration/autoland/rev/cd8af175ae50
Part 5, BigInt<>I64 conversion for inlined calls r=lth

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7b6dbbb27b74
https://hg.mozilla.org/mozilla-central/rev/cd8af175ae50

Comment 15

[Noemi Erli[:noemi_erli]]

This was backed out on autoland since it started have high failure occurrences in js/src/jit-test/tests/wasm/bigint/bigint.js followed by application crashed [@ js::gc::IsInsideNursery]
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=sm&tochange=5797e768d878d00f2545fd94a88019abf2cd4a68&fromchange=15650b89e468574a18179bfac89aa4b12cecaa6a

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

Backout: https://hg.mozilla.org/integration/autoland/rev/c1b1ba2c99b1268ba658158daa200a8c79b1b8e7

Comment 16

[Noemi Erli[:noemi_erli]]

Backout of parts 1->3: https://hg.mozilla.org/integration/autoland/rev/92c42c6fb67dbaa16c2c157fa5c6a29f6ec772e6

Comment 17

[Lars T Hansen [:lth]]

I requested a backout of this patch set given the spike in failures when it was finally enabled and given the number of fixes that were requested in bug 1633714. I think that the latter patches should be folded into this patch set, we should re-review (TBD), and we should re-land after the code freeze for FF77 ends, ie in May sometime, but not too late in May. That way, this patch set can still make it into FF78 which is an ESR release.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backouts merged:
https://hg.mozilla.org/mozilla-central/rev/c1b1ba2c99b1
https://hg.mozilla.org/mozilla-central/rev/92c42c6fb67d

Comment 19

[Asumu Takikawa]

(In reply to Lars T Hansen [:lth] from comment #17)

I requested a backout of this patch set given the spike in failures when it was finally enabled and given the number of fixes that were requested in bug 1633714. I think that the latter patches should be folded into this patch set, we should re-review (TBD), and we should re-land after the code freeze for FF77 ends, ie in May sometime, but not too late in May. That way, this patch set can still make it into FF78 which is an ESR release.

Sorry for these intermittent crashes. I'll see if I can reproduce them locally and produce a test if possible too to go along with the fixes.

Just to make sure I understand, would it be best to squash the patches from bug 1633714 into patches on here, or would it be better to append the patches directly onto the patchset? If the former, I've pulled down the patches from phabricator and have a squashed patch I can upload after testing. Should I wait on some review in bug 1633714 first?

Comment 20

[Lars T Hansen [:lth]]

Provided that the patches from the other bug can be folded naturally into the five patches you have here then I think it would be better to squash the patches from the other bug into this one, since many/most of them are adjustments to the patches here. If that's not really practical then we should discuss further what we can do.

You should probably wait for review, but (a) Wingo's done I think and I hope to finish today, or tomorrow at the latest, and (b) anba's work is normally very solid and I don't expect many or even any changes.

Comment 21

[Marian Raiciof [:marauder]]

The backout from changeset c1b1ba.... caused the improvements below:

== Change summary for alert #25774 (as of Wed, 29 Apr 2020 17:00:04 GMT) ==

Improvements:

15% raptor-tp6-slides-firefox-cold loadtime linux64-shippable-qr opt 2,793.54 -> 2,371.92
15% raptor-tp6-slides-firefox-cold loadtime windows10-64-shippable-qr opt 2,776.79 -> 2,360.33
12% raptor-tp6-slides-firefox-cold loadtime windows10-64-shippable-qr opt 2,678.21 -> 2,368.00
4% raptor-tp6-slides-firefox-cold linux64-shippable-qr opt 1,393.47 -> 1,331.29
4% raptor-tp6-slides-firefox-cold windows10-64-shippable-qr opt 1,369.00 -> 1,312.34

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25774

Comment 22

[Asumu Takikawa]

Does this need any additional review before trying to re-land? I have updated some of the diffs to rebase up to recent changes and the patches from bug 1633714 are folded in now. When re-landing I'll also include the test patch from bug 1633740 that will check that the GC nursery issue is fixed.

Comment 23

[Lars T Hansen [:lth]]

I guess we can cross our fingers that Andre found the bugs that were findable and just land.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cc927f402b7
Part 1, BigInt<>I64 conversion for inlined calls r=wingo,lth
https://hg.mozilla.org/integration/autoland/rev/5a592b174ba3
Part 2, BigInt<>I64 conversion for inlined calls r=lth,wingo
https://hg.mozilla.org/integration/autoland/rev/ae0ddba53299
Part 3, BigInt<>I64 conversion for inlined calls r=lth,wingo
https://hg.mozilla.org/integration/autoland/rev/f036cb46d18f
Part 4, BigInt<>I64 conversion for inlined calls r=lth,wingo
https://hg.mozilla.org/integration/autoland/rev/648ab7aa47d3
Part 5, BigInt<>I64 conversion for inlined calls r=lth

Comment 25

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4cc927f402b7
https://hg.mozilla.org/mozilla-central/rev/5a592b174ba3
https://hg.mozilla.org/mozilla-central/rev/ae0ddba53299
https://hg.mozilla.org/mozilla-central/rev/f036cb46d18f
https://hg.mozilla.org/mozilla-central/rev/648ab7aa47d3

Comment 27

[Lars T Hansen [:lth]]

Currently blocks the P1 ship bug, so P1. Asumu, what's the status - are we good, and is this bug open only because of leave-open?

Comment 28

[Asumu Takikawa]

Yes, with the already merged patches this should be good. I think it's ok to close.