Closed
Bug 485959
Opened 16 years ago
Closed 16 years ago
TM: Recording continues across loop edge (probably involving tryBranchAfterCond)
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b4
People
(Reporter: jorendorff, Assigned: gal)
References
(Blocks 1 open bug)
Details
(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #370034 -
Attachment mime type: text/x-c → text/plain
Assignee | ||
Updated•16 years ago
|
Assignee: general → gal
Assignee | ||
Comment 3•16 years ago
|
||
Confirmed with TM tip.
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
Attachment #370527 -
Flags: review?(jorendorff)
Assignee | ||
Comment 7•16 years ago
|
||
review ping
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Reporter | ||
Comment 8•16 years ago
|
||
Comment on attachment 370527 [details] [diff] [review]
patch
Clearly correct now that I know what JSOP_LOOP is for! Sorry for the slow review.
Attachment #370527 -
Flags: review?(jorendorff) → review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: checkin-needed
Updated•16 years ago
|
Whiteboard: checkin-needed → checkin-needed, fixed-in-tracemonkey
Comment 9•16 years ago
|
||
Comment on attachment 370527 [details] [diff] [review]
patch
This patch breaks --disable-jit
Assignee | ||
Comment 10•16 years ago
|
||
Trivial build fix. Carrying forward review from jorendorff.
http://hg.mozilla.org/tracemonkey/rev/9ad17d099fdd
Whiteboard: checkin-needed, fixed-in-tracemonkey → fixed-in-tracemonkey
Reporter | ||
Comment 11•16 years ago
|
||
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).
Comment 12•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 13•16 years ago
|
||
(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
Comment 14•16 years ago
|
||
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•