507089 - TM/nanojit: prepare to add get/set methods for CallInfo::_argtypes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

CallInfo::_argtypes is a bitfield containing 9 x 3-bit fields (it was 9 x 2-bits until recently).  There are no get/set methods for this bitfield, so it's manipulated directly with shifting+masking in a number of places.  This is very error-prone, especially given that it's not obvious the order of the arguments (right-to-left, left-to-right?) nor where the return value is represented (first or last?)

I plan to rectify this.

Comment 1

[Nicholas Nethercote [inactive]]

The patch to lirasm in bug 519873 introduces some set functions.  That's a good start for this bug.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: NJ patch

This patch:

- Renames various things to talk about "types" rather than "sizes", because
  it's a more accurate description.  Eg. ArgSize -> ArgType, ARGSIZE_* ->
  ARGTYPE_*.  Similarly, CallInfo::_argtypes as CallInfo::_typesig.

- Introduces CallInfo::typeSig[012345678]() and typeSigN() for creating
  _typesigs, much less error-prone than explicit bit manipulations.
  In lirasm, these replace the inferior pre-existing ones.

- Gets rid of the too-clever-by-half ARGSIZE_MASK_INT.  In the few places
  where it was used, it now just checks the integer possibilities in turn.
  Got rid of _count_args() accordingly.  This means the ArgType enum values
  can be sequential.

- Replaces the (unchecked) table-base opcode selection in
  LirBufWriter::insCall() with a (checked) switch.

- Moved (and renamed) get_sizes() and count_args() from Assembler.cpp into
  LIR.cpp, with the rest of the CallInfo stuff.
  
- Removed the (AFAICT) unused '...' args from the _nvprof macro because
  they cause lots of warnings like this from GCC:
  
    ../nanojit/CodeAlloc.cpp:466:39: warning: ISO C99 requires rest arguments
    to be used

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: TM patch

This patch updates TM for the above NJ changes.  It also reformats
JS_DEFINE_CALLINFO_* to be easier to read.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: TR patch

This patch updates TR for the above NJ changes.  It keeps the SIG0..SIG8
macros, but defines them in terms of typeSig0()..typeSig8().

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/8075a19e1156

Comment 6

[Nicholas Nethercote [inactive]]

NJ part: http://hg.mozilla.org/tracemonkey/rev/adc7e42ecbe3
TM part: http://hg.mozilla.org/tracemonkey/rev/3c673457c90b

Comment 7

[Nicholas Nethercote [inactive]]

These got backed out due to mysterious windows bustage.

http://hg.mozilla.org/projects/nanojit-central/rev/8a1ae643bab0
http://hg.mozilla.org/tracemonkey/rev/636964f611b3

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/adc7e42ecbe3

Comment 9

[Nicholas Nethercote [inactive]]

This one was backed out... Sayre, I hope you merged the backouts as well!

Comment 10

[Nicholas Nethercote [inactive]]

The problem that required the backout was mysterious Window bustage that was, as far as I could tell (with Julian's help), an MSVC bug.

To keep things moving, I'm going to reland a subset of this bug:

- Renames various things to talk about "types" rather than "sizes", because
  it's a more accurate description.  Eg. ArgSize -> ArgType, ARGSIZE_* ->
  ARGTYPE_*.  Similarly, CallInfo::_argtypes as CallInfo::_typesig.

- Gets rid of the too-clever-by-half ARGSIZE_MASK_INT.  In the few places
  where it was used, it now just checks the integer possibilities in turn.
  Got rid of _count_args() accordingly.  This means the ArgType enum values
  can be sequential.

- Moved (and renamed) get_sizes() and count_args() from Assembler.cpp into
  LIR.cpp, with the rest of the CallInfo stuff.

It seemed to the the typeSigN functions that caused the MSVC bug, and the other pieces removed ended up landing in other patches.

I've filed bug 553962 as a follow-up for the rest of it.

Comment 11

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/55f02d797675
http://hg.mozilla.org/projects/nanojit-central/rev/2ad8e20152c9

http://hg.mozilla.org/tracemonkey/rev/cc5f301065a7
http://hg.mozilla.org/tracemonkey/rev/d64155596297
http://hg.mozilla.org/tracemonkey/rev/f4e6161afc35

(That includes a Windows bustage fix-up patch.  Who knew that "VOID" was #defined on Windows as "void"?  Not me.)

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: subset TR patch

Comment 13

[Edwin Smith]

TR patches
http://hg.mozilla.org/tamarin-redux/rev/c53c3642bbeb
http://hg.mozilla.org/tamarin-redux/rev/7970a79e183a

Comment 14

[Nicholas Nethercote [inactive]]

I'm 99.99% certain this has been merged to mozilla-central.