Closed Bug 507089 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(3 files, 1 obsolete file)

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.
The patch to lirasm in bug 519873 introduces some set functions.  That's a good start for this bug.
Blocks: 525815
Attached patch NJ patchSplinter Review
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
Attachment #423455 - Flags: review?(edwsmith)
Attached patch TM patchSplinter Review
This patch updates TM for the above NJ changes.  It also reformats
JS_DEFINE_CALLINFO_* to be easier to read.
Attachment #423456 - Flags: review?(jorendorff)
Attached patch TR patch (obsolete) — Splinter Review
This patch updates TR for the above NJ changes.  It keeps the SIG0..SIG8
macros, but defines them in terms of typeSig0()..typeSig8().
Attachment #423457 - Flags: review?(edwsmith)
Attachment #423456 - Flags: review?(jorendorff) → review+
Attachment #423457 - Flags: review?(edwsmith) → review+
Attachment #423455 - Flags: review?(edwsmith) → review+
NJ part: http://hg.mozilla.org/tracemonkey/rev/adc7e42ecbe3
TM part: http://hg.mozilla.org/tracemonkey/rev/3c673457c90b
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/adc7e42ecbe3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This one was backed out... Sayre, I hope you merged the backouts as well!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Blocks: 553962
Summary: TM/nanojit: introduce get/set methods for CallInfo::_argtypes → TM/nanojit: prepare to add get/set methods for CallInfo::_argtypes
Attached patch subset TR patchSplinter Review
Attachment #423457 - Attachment is obsolete: true
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
I'm 99.99% certain this has been merged to mozilla-central.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.