804636 - Consider decomposing LOCAL/ARG inc/dec ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Handling these in the JITs is quite complicated, especially with IonMonkey: compilation aborts if the input is not known to be int32/double, since we can't bailout in the middle of a MToInt32/MToDouble + MAdd sequence.

Decomposing these like the other INC/DEC ops lets us remove at least 300 lines of code, mostly from the JITs. Main downside is that it increases bytecode size (4 to 15 ops).

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> 
> Main downside is that it increases
> bytecode size (4 to 15 ops).

Eh s/ops/bytes of course.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1: Decompose LOCAL/ARG inc/dec

15 files changed, 31 insertions(+), 548 deletions(-)

Will post another patch to remove the loop test analysis from LoopState, since this patch makes it useless.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1: Decompose LOCAL/ARG inc/dec, v2

Just some minor changes.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 675156 [details] [diff] [review]
Part 1: Decompose LOCAL/ARG inc/dec, v2

Review of attachment 675156 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Can you make sure though that this doesn't break the decompiler, so that the JIT inspector is still usable?  Unfortunately I think the inspector is currently the only way to exercise the decompiler, so you'd need to use the addon a bit and make sure it behaves right on scripts with inc/dec ops.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2: Fix the decompiler

I managed to make the decompiler work in the shell again, this patch fixes it.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 3: Remove loop test analysis

I will check some benchmarks before landing this.

Comment 7

[Jan de Mooij [:jandem]]

Part 1 + 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4204b2b987c

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 4: Remove more code

Removes another 65 lines I forgot in the first patch: TypeOracle::incslot and special handling for inc/dec ops in TypeDynamicResult.

Comment 9

[Jan de Mooij [:jandem]]

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/43e09a8466c2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43e09a8466c2

Comment 12

[Jan de Mooij [:jandem]]

And part 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3dcaf02c5e5

Comment 13

[Phil Ringnalda (:philor)]

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