501232 - nanojit: remove LIR_2

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

As far as I can tell, LIR_2 is a hack that exists only because the LIR representation currently allows a maximum of two operands, but LIR_cmov and LIR_qcmov need three operands.

Once variable-width LIR is landed, we can add a LInsOp3 instruction class and LIR_2 will be able to be removed.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

Patch removing LIR_2.

Pros:
- No special handling required for LIR_2.
- One fewer opcodes.
- Number of LIR instructions generated for SunSpider drops 1.5%.
- Size of LIR generated for SunSpider drops 1.1%.

Cons:
- Increases source code size by 95 lines, much of it boilerplate (eg. ins3(),
  hash3(), find3(), LInsOp3 methods.
- Increases size of 'js' binary by 49 bytes on x86/Linux.

The performance impact is surprising:  I get a 10ms slowdown for SunSpider on Mac, entirely within string-unpack-code.js.  But I get an 8ms speedup on Linux, spread all over.  Cachegrind simulates a 65% increase in I1 read misses which is enormous (I've never seen anything like it, even for much bigger changes than this one) but I suspect it's a transient thing and some tiny change will soon be made that undoes this effect.  If this passes review, I'll look again into the performance after some more commits have occurred to see if my suspicions about transience are confirmed.

Comment 2

[Julian Seward [:jseward]]

> Patch removing LIR_2.

+1 for this.  LIR_2 also complicates and hinders typechecking of LIR,
since one would have to represent pair types in order to deal with it
properly.  So getting rid of it is a Good Thing imo.

> spread all over.  Cachegrind simulates a 65% increase in I1 read misses which
> is enormous (I've never seen anything like it, even for much bigger changes
> than this one)

Odd.  A couple of things come to mind:

- change in inlining behaviour?  crossing some invisible do/don't-inline
  threshold in gcc?

- you're simulating a sufficiently associative cache?  In particular
  direct-mapped caches tend to have nasty spikes like this if you hit
  a bad aliasing case.

Comment 3

[Robert Sayre]

(In reply to comment #1)
> The performance impact is surprising:  I get a 10ms slowdown for SunSpider on
> Mac, entirely within string-unpack-code.js.

What say shark?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> (In reply to comment #1)
> > The performance impact is surprising:  I get a 10ms slowdown for SunSpider on
> > Mac, entirely within string-unpack-code.js.
> 
> What say shark?

Very little -- nanojit barely registers for string-unpack-code.js because it spends almost all time in the interpreter.

But today I'm getting a ~3ms speedup, having updated.  I think it's just noise.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Robert Sayre]

had to back this out because of test failures. probably not due to this patch, so it will go back in soon.

Comment 7

[Robert Sayre]

this patch has a conflict in the opcode.tbl. could you land a rebased patch?

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: rebased patch

Comment 9

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/8877e1f8645b

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8877e1f8645b

Comment 11

[Andreas Gal :gal]

This has to be backed out too. Crash on browser startup with TMFLAGS=full and also likely crash in other places (liveness analysis pass). LIR_2 was removed, but cmov still does oprnd2()->oprnd1(), ->oprnd2()->oprnd2() instead of oprnd2(), oprnd3().

Comment 12

[Andreas Gal :gal]

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

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: new patch that fixes problems

This patch should fix the problems.  I was able to reproduce the problems in live() and formatIns() with a TMFLAGS=full shell run.  (I can't get TMFFLAGS to work with the browser -- is there anything special required?)

The insIsS16() problem I couldn't test because the normal tests don't trigger that code -- it only would be encountered in very rare circumstances.  I found it when reviewing all the LIR_cmov occurrences.

I also updated the PPC backend, but didn't test that.

Comment 14

[Andreas Gal :gal]

Comment on attachment 389643 [details] [diff] [review]
new patch that fixes problems

TMFLAGS=x should work in the browser. Make sure you built the browser with --enable-debug.

Comment 15

[Nicholas Nethercote [inactive]]

3rd time lucky:

http://hg.mozilla.org/tracemonkey/rev/2973e9c44842

Sayre, for m-c you'll have to back out 8877e1f8645b (ie. merge d75d6cd53041) before you can merge this one.  Sorry for the mess.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2973e9c44842