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.

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.

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

The backout looks fine to me.

This should fix the problems.

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.

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

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

2nd attempt: http://hg.mozilla.org/tracemonkey/rev/bb978b24ce88