Closed
Bug 851490
Opened 11 years ago
Closed 11 years ago
BaselineCompiler: Investigate Windows PGO failures
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
2.65 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
PGO builds on the BC branch fail jsreftests sometimes, see for instance [1]. This also seems to break Kraken crypto-aes in the browser. Looking at the failing tests it's probably related to the bitwise operators, ToInt32 or Math.pow (we've had PGO problems with Math.pow before..) I will try to reduce this now... [1] https://tbpl.mozilla.org/php/getParsedLog.php?id=20649469&tree=Ionmonkey&full=1
Assignee | ||
Comment 1•11 years ago
|
||
I debugged one of these failures. A minimal testcase is like this: for (var i=0; i<10; i++) {} // Warm up for baseline. var x = 0; var y = 1.2 >>> x; With a PGO build y is 0 instead of 1. The problem seems to be in DoBinaryArithFallback. The code is like this: switch (op) { ... case JSOP_URSH: { if (!UrshOperation(cx, script, pc, lhs, rhs, ret.address())) return false; break; } ... } MSVC does some heavy inlining and PGO here, it inlines UrshOperation -> ToUint32 -> ToUint32Slow -> ToUint32. Annotated code for the JSOP_URSH case: // load lhs type in ecx 6e33d9e0 8b4804 mov ecx,dword ptr [eax+4] // Save pointer to rhs in ebx 6e33d9e3 8bda mov ebx,edx // load lhs payload in edx 6e33d9e5 8b10 mov edx,dword ptr [eax] // jump if lhs is int32 6e33d9e7 83f981 cmp ecx,0FFFFFF81h 6e33d9ea 0f845d050000 je mozjs!+0x1a4d (6e33df4d) // jump if lhs is not double 6e33d9f0 83f980 cmp ecx,0FFFFFF80h 6e33d9f3 0f873d521400 ja mozjs!+0x3b6c6 (6e482c36) // Load lhs Value, pass as double. Code generated here is wrong: // The debugger shows that the LHS value is at esp+40h. // esp+48h is the address after performing the "sub esp, 8" 6e33d9f9 dd442448 fld qword ptr [esp+48h] 6e33d9fd 83ec08 sub esp,8 6e33da00 dd1c24 fstp qword ptr [esp] // call ToIntWidth<32, int32_t>. 6e33da03 e8c8590700 call mozjs!+0xcf0 (6e3b33d0) 6e33da08 83c408 add esp,8 // save result. 6e33da0b 89442450 mov dword ptr [esp+50h],eax // load rhs type and payload 6e33da0f 8b0b mov ecx,dword ptr [ebx] 6e33da11 8b5b04 mov ebx,dword ptr [ebx+4] // jump if rhs is not int32 6e33da14 83fb81 cmp ebx,0FFFFFF81h 6e33da17 0f853b521400 jne mozjs!+0x3b6e8 (6e482c58) // Perform x >>> (y & 0x1f) 6e33da1d 8b442450 mov eax,dword ptr [esp+50h] 6e33da21 83e11f and ecx,1Fh 6e33da24 d3e8 shr eax,cl // Check for INT32_MIN. 6e33da26 3dffffff7f cmp eax,7FFFFFFFh ...
Assignee | ||
Comment 2•11 years ago
|
||
Oh and I also looked at a "green" PGO build. In that case MSVC emitted different code, it did not inline ToUint32Slow.
Assignee | ||
Comment 3•11 years ago
|
||
Disable PGO for DoUnaryArithFallback and DoBinaryArithFallback (see comment 1).
Attachment #725505 -
Flags: review?(dvander)
Comment on attachment 725505 [details] [diff] [review] Patch Review of attachment 725505 [details] [diff] [review]: ----------------------------------------------------------------- What's that? A bug introduced by PGO, you say? That's never happened before!
Attachment #725505 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/5f25ee4c94ab Problem is that PGO builds fail frequently but not always, so I will leave this bug open for a week or so to see if there are still failures.
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > Problem is that PGO builds fail frequently but not always, so I will leave > this bug open for a week or so to see if there are still failures. All PGO builds are green so far.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•