BaselineCompiler: Investigate Windows PGO failures

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 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

5 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

5 years ago
Created attachment 725505 [details] [diff] [review]
Patch

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

5 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

5 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 858566
Depends on: 873296
You need to log in before you can comment on or make changes to this bug.