932757 - Remove JSOP_POPV and JSOP_STOP

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

The following are equivalent (in what they do):
JSOP_POPV and JSOP_SETRVAL
JSOP_STOP and JSOP_RETRVAL

So it makes sense to merge them.

I choose to use JSOP_SETRVAL instead of JSOP_POPV, since the name JSOP_SETRVAL gives more information. 
I choose to use JSOP_STOP instead of JSOP_RETRVAL, since JSOP_STOP is also used when JSOP_SETRVAL isn't used. So it made more sense to me (it returns undefinedValue in that case).

Most changes are trivial (just remove the other opcode). Except for baseline. Here JSOP_STOP was used to know the end of the script. So I had to change that to test script->code + script->length instead

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Hannes Verschore [:h4writer]]

Just had a discussion on IRC with jandem,

He would prefer JSOP_RETRVAL and drop JSOP_STOP.
Reason: it seems strange to see JSOP_STOP in the middle of a script.

For me it is the same. My initial thought was to use JSOP_STOP based on my explanation above. But I can change to JSOP_RETRVAL if others think it is better...

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 824605 [details] [diff] [review]
Patch

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

OK, looks good.

::: js/src/jit/BaselineCompiler.cpp
@@ +688,2 @@
>                  break;
>              pc += GetBytecodeLength(pc);

Maybe swap the order of the if-statement and the `pc +=` statement, to avoid calling GetBytecodeLength twice.

@@ +742,3 @@
>              break;
>  
>          pc += GetBytecodeLength(pc);

Same here.

Comment 4

[Hannes Verschore [:h4writer]]

So in the end I removed jsop_stop (RIP) and we now use jsop_retrval instead. Since Jan and Jason both thought it made more sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cdec2743961

Comment 5

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/0cdec2743961