842419 - Clean up some front-end cruft

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I have various patches coming shortly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove unnecessary JSOp arg from EmitBackPatchOp().

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unnecessary SRC_CONTINUE notes, and kill off PNX_ENDCOMMA.

The only remaining use of SRC_CONTINUE is IonMonkey, and it only needs it on
JSOP_GOTO.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove dead JOF_* constants, and kill off JSOP_BACKPATCH_POP.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Fix up js_SrcNoteSpec[].

We forgot to update this in bug 838813.  I also numbered the elements for
easier reading, and used "foo/bar" for the cases where the srcnote numbers
overlap.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unnecessary SRC_CONTINUE notes, and kill off PNX_ENDCOMMA.

I forgot to update the SRC_CONTINUE comment.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 715290 [details] [diff] [review]
(part 4) - Fix up js_SrcNoteSpec[].

I have a better version of this coming up.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Clean up srcnote constants and js_SrcNoteSpec.

This patch:
- Defines the source note values in a more sensible order, grouping related
  ones together.
- Stops the sharing of some of the values, which is just confusing.
- Numbers the entries in js_SrcNoteSpec[] for easier reading.
- Makes SN_IS_GETTABLE more robust.

There are now 4 unused values before SRC_XDELTA.  I tried changing SRC_XDELTA
from 24 to 20 but this causes all sorts of test failures, and I haven't worked
out why.  Mildly terrifying...

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Reduce the arity of SRC_IF_ELSE from 2 to 1.

SRC_IF_ELSE's first operand is used by IonMonkey.  It's second isn't used.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Reduce the arity of SRC_IF_ELSE from 2 to 1.

I missed a spot.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Reduce the arity of four loop-related srcnotes.

This patch reduces the arity of four loop-related srcnotes.  This avoids
emitting some atom indices, which is nice.

Note that SRC_FOR_IN's 2nd offset has become its 1st, and so its layout matches
SRC_WHILE's layout -- that explains the change in whileOrForInLoop().

Comment 11

[Nicholas Nethercote [inactive]]

> There are now 4 unused values before SRC_XDELTA.  I tried changing SRC_XDELTA
> from 24 to 20 but this causes all sorts of test failures, and I haven't
> worked out why.  Mildly terrifying...

Oh, now I get it.  24 is 11000b, and it's XDELTA if the top 2 bits are set.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Split SRC_SWITCH in two.

TABLESWITCH only needs 1 offset, but CONDSWITCH needs 2.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Reduce the arity of SRC_CATCH from 1 to 0.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Reduce the arity of SRC_CATCH from 1 to 0.

Oh, we don't need SRC_CATCH on JSOP_ENTERBLOCK.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 715320 [details] [diff] [review]
(part 4) - Clean up srcnote constants and js_SrcNoteSpec.

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

::: js/src/frontend/BytecodeEmitter.h
@@ +247,5 @@
>   * enum, so its initializers need to match the order here.
>   *
> + * It's possible for two source note kinds to share the note type value if they
> + * have the same arity and cannot both apply to the same opcode.  But please
> + * don't do that -- it's just confusing.

Agreed, but how about we just delete this paragraph? I think all current maintainers are on the same page.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 715330 [details] [diff] [review]
(part 5) - Reduce the arity of SRC_IF_ELSE from 2 to 1.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3905,5 @@
>      } else {
>          /*
>           * We came here from the goto further below that detects else-if
>           * chains, so we must mutate stmtInfo back into a STMT_IF record.
> +         * Also we need a note offset for SRC_IF_ELSE to help IonMonkey.

That goto: (è_é)

r=me if you're willing to change it to tail-recurse instead; the relevant unit test is
  tests/js1_5/Regress/regress-96526-003.js

::: js/src/shell/js.cpp
@@ +1747,1 @@
>              break;

You don't need a Sprintf here if you don't want one, just reuse the one below (case SRC_COND et al).

Otherwise: it'll fit on one line.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 715332 [details] [diff] [review]
(part 6) - Reduce the arity of four loop-related srcnotes.

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

Yes please!

I think SRC_CONT2LABEL is equivalent to SRC_CONTINUE with this change. Please unify!

(Unfortunately there is some sort of minor difference between SRC_BREAK and SRC_BREAK2LABEL, but it looks like maybe they could be unified too, with a bit more work.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +649,5 @@
>          return -1;
>  
> +    if (noteType != SRC_NULL)
> +        if (NewSrcNote(cx, bce, noteType) < 0)
> +            return -1;

Style nit: Braces on the outer if-statement, please.

Is the followup already on file to use opcodes rather than source notes for this?

::: js/src/ion/IonBuilder.cpp
@@ +2255,5 @@
>      //  3/ Generate code for all bodies (see processCondSwitchBody).
>  
>      JS_ASSERT(JSOp(*pc) == JSOP_CONDSWITCH);
>      jssrcnote *sn = info().getNote(cx, pc);
> +    JS_ASSERT(SN_TYPE(sn) == SRC_SWITCH);

Thank you so much for adding these assertions. It's more than bold going commando to a source-notes dance; it's crazy. What were the original authors thinking?! ;)

::: js/src/shell/js.cpp
@@ +1755,1 @@
>              break;

Super-micro-nit: If you prefer to have these cases explicit here, might as well remove the default:; case and fill in the other missing source notes. Your call.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 715463 [details] [diff] [review]
(part 7) - Split SRC_SWITCH in two.

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

Sure, OK.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2329,3 @@
>      if (switchOp == JSOP_CONDSWITCH) {
>          /*
>           * 0 bytes of immediate for unoptimized ECMAv2 switch.

get rid of "ECMAv2" here while you're in the neighborhood?

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 715867 [details] [diff] [review]
(part 8) - Reduce the arity of SRC_CATCH from 1 to 0.

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

Nice. Now that I know ReconstructPCStack is the only code using SRC_CATCH, I'll take a look and see if we can get rid of it too.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3610,1 @@
>          /* ifeq <next block> */

Pre-existing style nit: comments get a blank line before (except at the top of a file or block).

Comment 20

[Nicholas Nethercote [inactive]]

> Is the followup already on file to use opcodes rather than source notes for
> this?

Not yet.  I'm still mulling over exactly how many srcnotes can be handled in this way.  But it's on my todo list.

Comment 21

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a01991f5219c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ad1347a31a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ff88dac40a
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a6fd76e428
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e43753805f
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c652c92bc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40a568d6929
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a91fda6743

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a01991f5219c
https://hg.mozilla.org/mozilla-central/rev/f2ad1347a31a
https://hg.mozilla.org/mozilla-central/rev/c7ff88dac40a
https://hg.mozilla.org/mozilla-central/rev/75a6fd76e428
https://hg.mozilla.org/mozilla-central/rev/57e43753805f
https://hg.mozilla.org/mozilla-central/rev/88c652c92bc8
https://hg.mozilla.org/mozilla-central/rev/c40a568d6929
https://hg.mozilla.org/mozilla-central/rev/14a91fda6743