The three highlighted lines in http://pastebin.mozilla.org/638391 are the same. This means the recorder is unrolling a loop, which I'm sure is not intentional. I suspect the patch in bug 482800 regressed this; fuseIf should be detecting a loop edge here. No reproducible test case yet, but I think I can make one.

The spew from this script includes: -- 00177: 80 lt i2f178 = i2f add166 st sp[152] = add166 st sp[136] = add166 st sp[152] = add166 st sp[160] = 3 stq sp[168] = $<anonymous>.on fmul47 = fmul #0x40080000:0, $<anonymous>.on stq sp[160] = fmul47 flt53 = flt i2f178, fmul47 xf951: xf flt53 -> pc=0x30e22d imacpc=0x0 sp+168 rp+8 00110: 81 getlocal 0 -- (This test case is sensitive to the stream of random numbers: if you change the 3 on the first line to a 1, the bug goes away.) I don't know how serious this bug is. It sort of has the effect of disabling the jit, which is how I noticed it.

I'm going to nominate this in case the bug is bad.

Confirmed with TM tip.

We blacklist a loop (header has a NOP, not a LOOP), so js_MonitorLoopEdge doesn't get called, and as a result js_RecordLoopEdge is not called. Attached patch fixes that. No perf impact.

b+ please

review ping

Comment on attachment 370527 [details] [diff] [review] patch Clearly correct now that I know what JSOP_LOOP is for! Sorry for the slow review.

Comment on attachment 370527 [details] [diff] [review] patch This patch breaks --disable-jit

Trivial build fix. Carrying forward review from jorendorff. http://hg.mozilla.org/tracemonkey/rev/9ad17d099fdd

A little late, but: since NOP indicates the loop has been blacklisted, should we abort instead of calling js_MonitorLoopEdge in that case? BRANCH gets used a lot. I wonder if it would be a win or a loss to make BRANCH just `pc += (n); goto branch;` and put the common code there. Pro: code size; Con: lose CPU branch prediction benefit of having separate DO_OP() sites (on gcc, icc).

(In reply to comment #11) > A little late, but: since NOP indicates the loop has been blacklisted, should > we abort instead of calling js_MonitorLoopEdge in that case? Andreas should comment here. > BRANCH gets used a lot. I wonder if it would be a win or a loss to make BRANCH > just `pc += (n); goto branch;` and put the common code there. Pro: code size; > Con: lose CPU branch prediction benefit of having separate DO_OP() sites (on > gcc, icc). Igor and I have observed this branch prediciton benefit of separate branch instructions can be significant. We shouldn't worry about code size here, it is worse in other areas by far. In general, code size in our C++ memory usafe codebase is the riper target than in the JS VM. /be