493125 - nanojit: remove LIR_cs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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

[Edwin Smith]

(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.

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch removing LIR_cs

Comment 4

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/cdc4a1b07287

Comment 5

[Robert Sayre]

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