1618198 - Implement all JSOps supported by IonBuilder in WarpBuilder

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 1 - Pass BytecodeLocation by value. r?iain!

BytecodeLocation contains just a pointer in non-debug builds so
passing it by value is a bit more efficient. It's also more readable
without the const&.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 2 - Implement various trivial bytecode ops. r?iain!

Depends on D64368

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 3 - Implement more trivial ops. r?iain!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8246b1f7401d
part 1 - Pass BytecodeLocation by value. r=iain
https://hg.mozilla.org/integration/autoland/rev/d32df20ac97e
part 2 - Implement various trivial bytecode ops. r=iain

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69db56045db5
part 3 - Implement more trivial ops. r=iain

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8246b1f7401d
https://hg.mozilla.org/mozilla-central/rev/d32df20ac97e

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/69db56045db5

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 4 - Implement some effectful instructions. r?iain!

These ops are needed to support a typical for-loop. The next parts implement
the control flow ops for that.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 5 - Add MIRBuilderShared.h for some code that will be shared by IonBuilder and WarpBuilder. r?iain!

We won't be sharing a lot of actual builder code, but sharing these data
structures is easy and prevents name conflicts.

Depends on D64760

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 6 - Implement some control flow instructions. r?iain!

This is mostly based on the IonBuilder code, but it's slightly simpler because
we don't have to handle loop restarts.

Depends on D64761

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47d4208c3e98
part 4 - Implement some effectful instructions. r=iain
https://hg.mozilla.org/integration/autoland/rev/60526abd728b
part 5 - Add MIRBuilderShared.h for some code that will be shared by IonBuilder and WarpBuilder. r=iain
https://hg.mozilla.org/integration/autoland/rev/121bcf9e7ce6
part 6 - Implement some control flow instructions. r=iain

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 7 - Implement more branch instructions. r?iain!

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 8 - Implement more arithmetic/bitwise ops. r?iain!

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 9 - Implement GetArg and SetArg. r?iain!

This is based on the IonBuilder code that was tidied up last week in
bug 1380281 and bug 1478350.

Depends on D64975

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/47d4208c3e98
https://hg.mozilla.org/mozilla-central/rev/60526abd728b
https://hg.mozilla.org/mozilla-central/rev/121bcf9e7ce6

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5bf3456f5c9
part 8 - Implement more arithmetic/bitwise ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/f1f0ce0ee1bf
part 9 - Implement GetArg and SetArg. r=iain

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 10 - Implement some more ops. r?iain!

Also:

• Adds some asserts to ensure we don't create TemporaryTypeSets, fixes issues.
• Makes some JSScript methods const so that BytecodeLocation can pass a const script.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/caea2367d405
part 7 - Implement more branch instructions. r=iain

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a5bf3456f5c9
https://hg.mozilla.org/mozilla-central/rev/f1f0ce0ee1bf

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/caea2367d405

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 11 - Implement various ops. r?iain!

For JSOp::Pow we now use an IC in Ion.

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 12 - Implement JSOp::Arguments, add WarpOpSnapshot mechanism. r?iain!

For JSOp::Arguments we need to look up the realm-specific template object on the
main thread. To support that, this patch adds a mechanism for snapshotting
op-specific data in WarpOracle. WarpScriptSnapshot contains a linked list of
WarpOpSnapshots that WarpBuilder can then iterate over.

JSOp::RegExp now uses a snapshot for the hasShared flag. Later we could use the
same mechanism for capturing Baseline IC data.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1382d11f1946
part 10 - Implement some more ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/d6b54adac7b0
part 11 - Implement various ops. r=iain

Comment 24

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1382d11f1946
https://hg.mozilla.org/mozilla-central/rev/d6b54adac7b0

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3922d7eba99f
part 12 - Implement JSOp::Arguments, add WarpOpSnapshot mechanism. r=iain

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3922d7eba99f

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 13 - Implement aliased var ops and ObjWithProto. r?iain!

Comment 28

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 14 - Implement environment chain initialization. r?iain!

The previous patch introduced more jit-test failures because we didn't
initialize the environment chain yet (a good thing because it means we're
beginning to support non-trivial tests!).

WarpEnvironment is added to store information WarpBuilder needs to build the
environment chain. The goal is to make most of the decisions in WarpOracle so
that the WarpBuilder code can be as simple as possible.

Depends on D66386

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06500ac5da83
part 13 - Implement aliased var ops and ObjWithProto. r=iain
https://hg.mozilla.org/integration/autoland/rev/8cd7ab297247
part 14 - Implement environment chain initialization. r=iain

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 15 - Implement iterator ops for for-in loops. r?iain!

Comment 31

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 16 - Support calls. r?iain!

For now this is the slowest possible implementation.

It reuses the CallInfo infrastructure. That class has some methods we probably
shouldn't use in WarpBuilder, but the class itself is still useful and we can
clean it up more when IonBuilder is gone.

This uncovered an issue with MCheckOverRecursed: IonBuilder gives this
instruction a resume point (it's not clear why) but WarpBuilder doesn't, so
DCE was eliminating it and some jit-test crashed with overrecursion.
Marking the instruction as guard fixes this.

Depends on D66738

Comment 32

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/06500ac5da83
https://hg.mozilla.org/mozilla-central/rev/8cd7ab297247

Comment 33

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 17 - Implement FunctionThis, GlobalThis, Get{G}Name, Bind{G}Name JSOps. r?iain!

Also deletes a comment in IonBuilder::jsop_getgname that no longer applies since
JitScript/BaselineInterpreter.

MBindNameCache had some unused fields.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84844a0d2aa
part 15 - Implement iterator ops for for-in loops. r=iain
https://hg.mozilla.org/integration/autoland/rev/45be0faa08c0
part 16 - Support calls. r=iain
https://hg.mozilla.org/integration/autoland/rev/5a989fc3e0f4
part 17 - Implement FunctionThis, GlobalThis, Get{G}Name, Bind{G}Name JSOps. r=iain

Comment 35

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 18 - Implement assorted JSOps. r?iain!

Comment 36

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/b84844a0d2aa
https://hg.mozilla.org/mozilla-central/rev/45be0faa08c0
https://hg.mozilla.org/mozilla-central/rev/5a989fc3e0f4

Comment 37

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 19 - Implement more ops. r?iain!

The change in GetIntrinsicValue is necessary. IonBuilder adds a resume point
after MCallGetIntrinsicValue but in WarpBuilder that doesn't work: because the
instruction is not effectful, it would trigger an assertion failure in resumeAfter.
Without its own resume point, however, we can't look up the current pc in
GetIntrinsicValue (the pc comes from the last resume point) so disable that code
if WarpBuilder is enabled.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e34098b876a7
part 18 - Implement assorted JSOps. r=iain

Comment 39

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 20 - Implement object/array allocation ops. r?iain!

For JSOp::Object we call setSingletonsAsValues in WarpOracle. There's a comment
for CompileRealm::setSingletonsAsValues claiming it's safe to do that off-thread
but it's simpler and safer to just avoid it.

Comment 40

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 21 - Implement object literal getter/setter ops. r?iain!

Comment 41

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e34098b876a7

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a03401c9f74e
part 19 - Implement more ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/c57e39cdd20f
part 20 - Implement object/array allocation ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/c9cb2b443d43
part 21 - Implement object literal getter/setter ops. r=iain

Comment 43

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 22 - Implement some more ops. r?iain!

Comment 44

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a03401c9f74e
https://hg.mozilla.org/mozilla-central/rev/c57e39cdd20f
https://hg.mozilla.org/mozilla-central/rev/c9cb2b443d43

Comment 45

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 23 - Implement InitProp* and InitElem* ops. r?iain!

This adds buildInitPropOp instead of reusing buildSetPropOp because there are
some subtle differences between Init and Set. Longer term we should consider
splitting the MIR and CacheIR code as well so it becomes easier to reason about.

Comment 46

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 24 - Implement JSOp::{Lambda,LambdaArrow,FunWithProto}. r?iain!

This requires some changes to LambdaFunctionInfo:

• Don't use LambdaFunctionInfo for MFunctionWithProto as the JSFunction is just passed to a C++ call.
• Make the callers pass the values to the constructor so that WarpBuilder can construct LambdaFunctionInfo from the snapshot.
• Use better static types now that we have FunctionFlags and BaseScript.
• Move the assert in LambdaFunctionInfo's constructor to CodeGenerator::visitLambda.

It's not clear if LambdaFunctionInfo is still required now that the function's
BaseScript* pointer is a constant, but long-term we probably want to move to a
model like this for all template objects anyway.

Comment 47

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 25 - Support FunCall/FunApply and spread calls. r?iain!

This needs more work to make it fast, especially JSOp::OptimizeSpreadCall.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f57ebbd9ff68
part 22 - Implement some more ops. r=iain

Comment 49

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f57ebbd9ff68

Comment 50

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 26 - Support Debugger and Instrumentation* ops. r?iain!

Note that the resumeAt in IonBuilder for JSOp::Debugger made little sense: MDebugger
bails out if an onDebuggerStatement hook is active but that uses the previous resume
point and not the one attached there. Strictly speaking MDebugger isn't effectful
at all so it doesn't need a resume point, but changing getAliasSet is probably not
worth it.

Unfortunately the instrumentation callback can be nursery allocated. For now
WarpOracle aborts compilation in this case, I think that's fine considering the
instrumentation machinery isn't really used anymore.

Finally, maybe in the future WarpOracle should allocate WarpScriptSnapshot upfront
so we don't need a constructor with many parameters.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9e0bd856b82
part 23 - Implement InitProp* and InitElem* ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/c447b127f99e
part 24 - Implement JSOp::{Lambda,LambdaArrow,FunWithProto}. r=iain
https://hg.mozilla.org/integration/autoland/rev/a86570b4deef
part 25 - Support FunCall/FunApply and spread calls. r=iain

Comment 52

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 27 - Implement JSOp::TableSwitch. r?iain!

This follows the IonBuilder code but the WarpBuilder design makes this come out
a bit simpler and nicer.

Also cleans up BytecodeLocation to not conflate jump targets and TableSwitch's
default-target. It worked, but it's an implementation detail and BytecodeLocation
should be explicit about the difference.

Comment 53

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f9e0bd856b82
https://hg.mozilla.org/mozilla-central/rev/c447b127f99e
https://hg.mozilla.org/mozilla-central/rev/a86570b4deef

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eddc9bce7b1
part 26 - Support Debugger and Instrumentation* ops. r=iain
https://hg.mozilla.org/integration/autoland/rev/e46d85e1e439
part 27 - Implement JSOp::TableSwitch. r=iain

Comment 55

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 28 - Support JSOp::Rest. r?iain!

Also folds MRestCommon into MRest. The MRestCommon base class was added for the
PJS version of MRest but PJS was removed a long time ago.

Comment 56

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 29 - Implement JSOp::Pos. r?iain!

Note that JSOp::Pos used to be very hot because it was used to implement the
increment and decrement operators, but for BigInt these were changed to use
JSOp::ToNumeric. JSOp::Pos is now only used for the +x operator.

IonBuilder compiles +x as x * 1, but that results in complexity elsewhere (ICs
and CacheIR inlining for example).

It's simpler to follow JSOp::ToNumeric/MToNumeric with an MToNumber instruction.
The patch adds the most basic implementation of this (still reasonably fast
because it checks for numbers inline). This also nicely matches what Baseline does.

Comment 57

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6eddc9bce7b1
https://hg.mozilla.org/mozilla-central/rev/e46d85e1e439

Comment 58

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 30 - Implement JSOp::{Try,Throw,ThrowSetConst}. r?iain!

Comment 59

[Jan de Mooij [:jandem]]

Attachment: Bug 1618198 part 31 - Turn JSOp whitelist into a blacklist. r?iain!

At this point WarpBuilder supports the same JSOps as IonBuilder.

We now get a compiler warning in WarpOracle and WarpBuilder when a new JSOp is
added but not handled in WarpOracle/WarpBuilder.

WarpOracle is slightly more efficient now because it has one switch-statement
instead of two.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bedd35b63bf
part 28 - Support JSOp::Rest. r=iain
https://hg.mozilla.org/integration/autoland/rev/58c3533fd95a
part 29 - Implement JSOp::Pos. r=iain

Comment 61

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1bedd35b63bf
https://hg.mozilla.org/mozilla-central/rev/58c3533fd95a

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4750f4a68ad1
part 30 - Implement JSOp::{Try,Throw,ThrowSetConst}. r=iain
https://hg.mozilla.org/integration/autoland/rev/b085cf5490fe
part 31 - Turn JSOp whitelist into a blacklist. r=iain

Comment 63

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4750f4a68ad1
https://hg.mozilla.org/mozilla-central/rev/b085cf5490fe