Closed Bug 501232 Opened 13 years ago Closed 13 years ago

nanojit: remove LIR_2

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #388187 - Flags: review?(edwsmith)
Summary: TM: remove LIR_2 → nanojit: remove LIR_2
> 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.
Attachment #388187 - Flags: review?(edwsmith) → review+
(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?
(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.
Whiteboard: fixed-in-tracemonkey
had to back this out because of test failures. probably not due to this patch, so it will go back in soon.
this patch has a conflict in the opcode.tbl. could you land a rebased patch?
Keywords: 4xp
Whiteboard: fixed-in-tracemonkey
Keywords: 4xp
Attached patch rebased patch (obsolete) — Splinter Review
Attachment #388187 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/8877e1f8645b
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/8877e1f8645b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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().
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #389033 - Attachment is obsolete: true
Attachment #389643 - Flags: review?(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.
Attachment #389643 - Flags: review?(gal) → review+
Whiteboard: fixed-in-tracemonkey
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.
http://hg.mozilla.org/mozilla-central/rev/2973e9c44842
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.