Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Working on a patch I found that a lot of nanojit's opcodes seem to be dead.  At least, on a debug x86 build I never encounter them (within LirReader::read()) running trace-test.js and the unit tests.  Here's a table summarizing what I found:

                                    // dead in TM       dead in tamarin-redux
                case LIR_addp:      // yes?             yes?
                case LIR_live:      // yes?             yes?
                case LIR_calli:     // yes              yes
                case LIR_j:         // yes?             yes?
                case LIR_ji:        // yes              yes 
                case LIR_ldc:       // yes?             yes?
                case LIR_qlo:       // yes?             yes?
                case LIR_qhi:       // yes?             yes?
                case LIR_cs:        // yes?             yes?
                case LIR_file:      // yes              yes?
                case LIR_line:      // yes              yes?
                case LIR_fret:      // yes?             yes?
                case LIR_fcalli:    // yes              yes
                case LIR_qiand:     // no               yes?
                case LIR_qiadd:     // no               yes?
                case LIR_qcmov:     // no               yes?
                case LIR_ldqc:      // yes?             yes?
                case LIR_qjoin:     // sf               sf?
                case LIR_qior:      // yes?             yes?
                case LIR_qilsh:     // no               yes?

Legend:
- The first column indicates if the opcode is (possibly) dead in TM, the second is for tamarin-redux.  
- "yes" means it's definitely dead, ie. there are no uses at all, based on using grep on all the *.cpp and *.h files.
- "yes?" means there are uses, but seemingly no defs of the opcode, so the uses are never hit
- "no" means it is live
- "sf" means it is used if soft_float is on (but soft_float appears to be broken, is this expected?)

I admit using grep isn't foolproof.  And maybe there are plans for some of the opcodes.  But I suspect at least some of these opcodes could be removed.  Anyone want to defend any of them?  

- I see from the wiki that #465582 proposes replacing LIR_ji with LIR_jtbl.

- Also, calli/fcalli are addressed by #492478;  that bug is currently stalled because the patch to remove them from TM might clash with prior patches that removed them from tamarin-redux.

- Some more of them might be used if soft_float is used;  I found qjoin is used that way, and if soft_float didn't break very quickly I might have hit others like qlo and qhi.

Comment 1

10 years ago
(In reply to comment #0)

updates:

                                     // dead in TM       dead in tamarin-redux
                 case LIR_addp:      // yes?             no
                 case LIR_live:      // yes?             no
                 case LIR_calli:     // yes              yes
                 case LIR_j:         // yes?             no
                 case LIR_ji:        // yes              yes 
                 case LIR_ldc:       // yes?             no
                 case LIR_qlo:       // yes?             sf + planned
                 case LIR_qhi:       // yes?             sf + planned
                 case LIR_cs:        // yes?             yes
                 case LIR_file:      // yes              no
                 case LIR_line:      // yes              no
                 case LIR_fret:      // yes?             no
                 case LIR_fcalli:    // yes              yes
                 case LIR_qiand:     // no               no
                 case LIR_qiadd:     // no               no
                 case LIR_qcmov:     // no               yes, but plan to use
                 case LIR_ldqc:      // yes?             no
                 case LIR_qjoin:     // sf               sf + planned
                 case LIR_qior:      // yes?             no
                 case LIR_qilsh:     // no               no

when grepping tamarin-redux, beware that we have 'p' aliases that
either map to qxxx or ixxx, that's probably why many of the 64bit
integer ops didn't show up.  most/all would also be used by 64bit TM.

> - I see from the wiki that #465582 proposes replacing LIR_ji with LIR_jtbl.

yes, it would be a rename since LIR_ji is not implemented.  of course
we could remove LIR_ji until LIR_jtbl lands.

> - Also, calli/fcalli are addressed by #492478;  that bug is currently stalled
> because the patch to remove them from TM might clash with prior patches that
> removed them from tamarin-redux.

I would go ahead with the opcode removal, be prepared
to re-introduce code in asm_call() when we re-merge.  no sense
trying to maintain dead code paths in TM's asm_call() until then.
(Assignee)

Comment 2

10 years ago
I was only looking in the tamarin-redux/nanojit/ directory, that's why I missed a lot of the uses.

LIR_ji is covered by bug 465582.  LIR_calli and LIR_fcalli are covered by bug bug 492478.  That leaves only LIR_cs to be removed.  So I'll rename this bug to cover just that opcode.
(Assignee)

Updated

10 years ago
Summary: nanojit: remove unnecessary opcodes → nanojit: remove LIR_cs
(Assignee)

Comment 3

10 years ago
Created attachment 379798 [details] [diff] [review]
patch removing LIR_cs
Attachment #379798 - Flags: review?(edwsmith)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED

Updated

10 years ago
Attachment #379798 - Flags: review?(edwsmith) → review+
(Assignee)

Updated

10 years ago
Whiteboard: checkin-needed

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: checkin-needed
(Assignee)

Comment 4

9 years ago
http://hg.mozilla.org/tracemonkey/rev/cdc4a1b07287
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/cdc4a1b07287
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.