504705 - TM/nanojit: use NULL as the condition for unconditional guards

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This is a cut-down version of the patch from bug 498807, which was controversial and has stalled.

Currently we always use insImm(1) as the condition for x/xbarrier/loop instructions.  This patch changes things so that we just use NULL instead.  It avoids creating quite a few unnecessary immediates and I'm seeing a 3--5ms speedup for SunSpider (but I don't entirely trust that number).

Furthermore, my real motivation for this is that the CseFilter needs some work, and all these unnecessary immediates are clouding my analysis of how effective CseFilter is.

Comment 1

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/692e8a1325f8

Comment 2

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/692e8a1325f8

Comment 3

[Andreas Gal :gal]

This crashes debug builds on startup with TMFLAGS=full.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000003
0x003d6799 in nanojit::LIns::opcode (this=0x0) at LIR.h:632
632	        inline LOpcode opcode()    const { return lastWord.opcode; }

There are several places where we don't null check operands (hash, live).

#3  0x003f7e59 in nanojit::live (gc=0x44be4b, frag=0x1b028bc0, logc=0x44be68) at ../../../js/src/nanojit/LIR.cpp:1578
1578	                    live.add(i->oprnd1(),i);
(gdb) list
1573	                    live.add(i->oprnd1(),i);
1574	                    live.add(i->oprnd2()->oprnd1(),i);
1575	                    live.add(i->oprnd2()->oprnd2(),i);
1576	                }
1577	                else if (operandCount[i->opcode()] == 1) {
1578	                    live.add(i->oprnd1(),i);
1579	                }
1580	                else if (operandCount[i->opcode()] == 2) {
1581	                    live.add(i->oprnd1(),i);
1582	                    live.add(i->oprnd2(),i);

Backing out.

Comment 4

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/080ff82bff5e

Comment 5

[Andreas Gal :gal]

I collided with a lirasm patch quite a bit. I hope I got the merge right. CCing jorendorff just to be sure.

Comment 6

[Nicholas Nethercote [inactive]]

The backout looks fine to me.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: patch that fixes problems

This should fix the problems.

Comment 8

[Andreas Gal :gal]

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

I don't think this is enough. LiveTable::live calls add() for the 2nd operand of xbarrier, for example, which is NULL, and add() does ->isconst() on it. There are a bunch of other places like that. The insImm() was there for a reason. We have to find all locations where we relied on this before and properly null-check. Might result in some ugly code here and there.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> LiveTable::live calls add() for the 2nd operand
> of xbarrier, for example, which is NULL, 

It doesn't.  xbarrier has an operandCount of 0 now, so LiveTable::live does nothing with it.

> and add() does ->isconst() on it.

You mean RetiredEntry::add()?  ->isconst() shouldn't be a problem, it just checks the opcode.

> There are a bunch of other places like that. The insImm() was there for a
> reason. We have to find all locations where we relied on this before and
> properly null-check. Might result in some ugly code here and there.

With the updated patch I can successfully run trace-test.js under the shell with opt, debug, and debug/TMFLAGS=full builds, and also the unit tests with a debug build.  Can you list any of these other places?  They're not being hit with the above tests.

Comment 10

[Andreas Gal :gal]

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

Ok, sounds good. Looks like xbarrier was the only weird case then with a middle argument being NULL.

Comment 11

[Nicholas Nethercote [inactive]]

x and loop also have NULL middle args, but their operandCounts were correctly 0 in LIRopcode.tbl.  Thanks for the review!

Comment 12

[Nicholas Nethercote [inactive]]

2nd attempt:

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

Comment 13

[Robert Sayre]

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