381973 - Declaring extra temporary slots in jsopcode.tbl

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently the emitter for post-increment bytecodes explicitly reserve an extra stack slot for GC roots via increasing stackMaxDepth if necessary. A similar reservation would be necessary for enditer bytecode to fix bug 349326. There I plan to use an extra slot to save an exception thrown from for-in loop when executing the iterator.

Thus I suggest to extend JSCodeSpec with a field indicating extra temporary slots that the bytecode may need and account for it with a generic code in UpdateDepth from jsemit.c.

Comment 1

[Igor Bukanov]

Attachment: implementation v1

The patch adds ntmps to JSCodeSpec and update jsopcode.tbl to insert another column to OPDEF macro. The patch always uses 0 for extra temporary slots and does not touch post-increment generation code as I plan to do that in a separated bug.

For greater readability in jsopcode.tbl I grouped the stack-related numbers without spaces as in:

OPDEF(JSOP_PUSH,      1,  "push",       NULL,         1,  0,1,0,  0,  JOF_BYTE)

I also considered writing OPDEF as

OPDEF(JSOP_PUSH,      1,  "push",       NULL,      1,   ST(0,1,0), 0,  JOF_BYTE)

where ST macro would be defined only when defining js_CodeSpec. But that, although very readable, makes jsopcode.tbl source even wider. 

The patch also always uses explicit strings when naming bytecode in OPDEF like "null" instead of refering to js_null_str as such names is used only in DEBUG-only dissembling routines.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 266041 [details] [diff] [review]
implementation v1

Does bitfield init work on MSVC?

I don't see any non-zero ntmps entries in jsopcode.tbl yet -- how many will there be? Could be overkill if a format flag could be used instead.

/be

Comment 3

[Igor Bukanov]

Attachment: implementation v2

The patch adds a new format flag to use with ops that needs an extra stack slot for rooting.

In the patch I changed JOF constants to use (1<<offset) for better readability.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 266981 [details] [diff] [review]
implementation v2

Looks ok. Nits:

* Traditional name is _SHIFT, not _LOG (maybe _LOG2 as runner-up to _SHIFT).

* JS_BIT(n) is a bit longer (three chars, vs. "1<<") but avoids uint vs. int and 16-bit int historical concerns.

No jsopcode.tbl patch yet?

/be

Comment 5

[Igor Bukanov]

Attachment: implementation v3

Addressing the nits: I can not use JS_BIT since the code has: 

#define JOF_NAME          (1U<<4) /* name operation */
#define JOF_PROP          (2U<<4) /* obj.prop operation */
#define JOF_ELEM          (3U<<4) /* obj[index] operation */
#define JOF_XMLNAME       (4U<<4) /* XML name: *, a::b, @a, @a::b, etc. */
#define JOF_VARPROP       (5U<<4) /* x.prop for arg, var, or local x */
#define JOF_MODEMASK      (7U<<4) /* mask for above addressing modes */

I.e. I need number << shift and JS_BIT hives only 1 << shift. But to make numbers unsigned I added the U prefix.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> 
> No jsopcode.tbl patch yet?

The post++/// changes will be done in bug 383188 and enditer starts to use the JOF_TMPSLOT in bug 349326.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 267186 [details] [diff] [review]
implementation v3

Looks good, thanks.

/be

Comment 8

[Igor Bukanov]

I committed the patch from comment 5 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.253; previous revision: 3.252
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.48; previous revision: 3.47
done