nanojit: kill operandCount

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
operandCount[] is a rich source of confusion and bugs.  See bug 495158 and bug 504705 for examples.

The first problem is that the meaning of operandCount isn't obvious, as LIR instructions can contain fields that you might think are "operands" but aren't really "operands", eg. GuardRecords in guards.

The second problem is that in functions that use operandCount[], some of the opcodes are named explicitly, but some are handled implicitly via the operandCount[].  This means that a lot of the operandCounts in LIRopcode.tbl are never actually used, and so there's no way of testing if they're correct.  Also, the mix of explicit/implicit handling makes it very easy to get things wrong when introducing new opcodes or changing existing ones.

Where operandCount[] is currently used -- LInsHashSet::hashcode(), LInshHashSet::equals() and LiveTable::live() are the important places, the rest are just assertions that specific operandCount[] values are sensible -- I propose changing the switches to just use opcode names explicitly.  The switches will be longer, but operandCount[] and the operandCount field in LIRopcode.tbl will be removed.  And the switches will be much less error-prone.
(Assignee)

Comment 1

9 years ago
The patch in bug 502778 gets rid of the worst uses of operandCount[];  after that's landed there'll just be some mopping up.
Depends on: 502778
(Assignee)

Comment 2

9 years ago
assemble_general() in lirasm is another offender.  It can be fixed by handling all opcodes directly in the big switch in assembleFragment(), instead of handling some directly and some via operandCount[].
(Assignee)

Updated

9 years ago
Depends on: 525413
(Assignee)

Comment 3

9 years ago
Created attachment 413250 [details] [diff] [review]
patch

assemble_general() was fixed elsewhere.

This patch cleans up the remaining uses.

- It removes the now-unused 'operands' argument from LIRopcode.tbl and all
  the OPDEF macros.

- It removes operandCount[].

- It uses a big switch in live() instead of operandCount[].  There are no
  performance concerns as live() is debug printing code.

- It removes the now-defunct assertions about operand counts in CseFilter::ins*().

- Since this touched every opcode line in LIRopcode.tbl, I took the chance
  to make some other minor modifications to that file that made it easier to
  apply consistent whitespace formatting:

  - Renamed OPDEF64 and OPD64, which is the same length as OPDEF.  (Bug
    504507 will merge them anyway.)
    
  - Used short names like "__3" and "__24_64" instead of the longer "unused3"
    and "unused24_64" for unused opcodes.

I tested the code by comparing the output of TMFLAGS=liveness before and
after the patch was applied on some big programs.  The output was the same.
(This required the tweak discussed in bug 528857.)
Attachment #413250 - Flags: review?(graydon)
(Assignee)

Comment 4

9 years ago
Created attachment 413251 [details] [diff] [review]
patch, v2
Attachment #413250 - Attachment is obsolete: true
Attachment #413251 - Flags: review?(graydon)
Attachment #413250 - Flags: review?(graydon)
Comment on attachment 413251 [details] [diff] [review]
patch, v2

I'm fine with it, but it seems a little more than a superficial change, tagging edwin to approve also.

(Also needs a trivial lirasm hunk)
Attachment #413251 - Flags: review?(graydon)
Attachment #413251 - Flags: review?(edwsmith)
Attachment #413251 - Flags: review+
(Assignee)

Comment 6

9 years ago
Created attachment 413963 [details] [diff] [review]
patch v3, includes required lirasm change
Attachment #413251 - Attachment is obsolete: true
Attachment #413963 - Flags: review?(edwsmith)
Attachment #413251 - Flags: review?(edwsmith)

Updated

9 years ago
Attachment #413963 - Flags: review?(edwsmith) → review+

Comment 8

9 years ago
http://hg.mozilla.org/tracemonkey/rev/00d38ab1a157
http://hg.mozilla.org/tamarin-redux/rev/2dc47ba046ee
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
(Assignee)

Comment 9

9 years ago
Tamarin needs an additional patch for a single use of OPDEF/OPDEF64 in CodegenLIR.cpp.  So Tamarin is currently broken.  It's a good idea to test before committing a change.  Generally Mozilla people merge NJ to TM, and Adobe people merge to TR;  I'm not sure where Sun people fit in there...

Comment 10

9 years ago
We are working on both tracemonkey and tamarin.
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/00d38ab1a157
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.