Closed Bug 1608771 Opened 7 months ago Closed 3 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: lth, Assigned: asumu)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.

Blocks: 1608772

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.

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.

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.

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.

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).

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.

Attachment #9130639 - Attachment is obsolete: true
Blocks: 1631702
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3588d7de8ea8
Part 1, BigInt<>I64 conversion for inlined calls r=wingo,lth
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91bc171d94e3
Part 2, BigInt<>I64 conversion for inlined calls r=lth,wingo
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02be9fd2d743
Part 3, BigInt<>I64 conversion for inlined calls r=lth,wingo
Pushed by apavel@mozilla.com:
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
Depends on: 1633714

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.

Flags: needinfo?(asumu)

(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?

Flags: needinfo?(asumu)

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.

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

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.

Flags: needinfo?(lhansen)

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

Flags: needinfo?(lhansen)
Pushed by rmaries@mozilla.com:
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
Duplicate of this bug: 1633714

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?

Flags: needinfo?(asumu)
Priority: P3 → P1

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

Flags: needinfo?(asumu)
Status: REOPENED → RESOLVED
Closed: 4 months ago3 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: mozilla77 → mozilla78
You need to log in before you can comment on or make changes to this bug.