Skip to content

Conversation

@Cellule
Copy link
Contributor

@Cellule Cellule commented Apr 13, 2016

The StElemI fast was ignoring the InvalidatedArrayLength bailout.
This change adds a verifies that the array length assumption is still valid and go to helper if not

It adds the first 2 instructions in the following lowered code where 1 in CMP s19.u32, 1 (0x1).u32 is the head segment bounds (ran using the added unittest).

 GLOBOPT INSTR:     [s6[Array][seg: s18][segLen: s19][>].var!+1].var = StElemI_A  0x00000001.var #001d  Bailout: #001d (BailOutOnImplicitCallsPreOp | BailOutOnInvalidatedArrayLength)


                       CMP            s19.u32, 1 (0x1).u32                    #
                       JBE            $L13                                    #
                       CMP            [s18.u32+20].var, 0x80000002.u32        #
                       JEQ            $L13                                    #
    [s18.u32+20].var = MOV            0x00000001.var                          #
                       JMP            $L12                                    #
$L13: [helper]                                                                #
                       PUSH           s6[Array][seg: s18][segLen: s19][>].var #
    s23(eax).u32    =  CALL           Array_Jit_GetArrayLength.u32            #
    s22.u32         =  MOV            s23(eax).u32                            #
                       PUSH           0 (0x0).i32                             #
                       PUSH           0xXXXXXXXX (ScriptContext).u32          #
                       PUSH           0x00000001.var                          #
                       PUSH           0x00000003.var                          #
                       PUSH           s6[Array][seg: s18][segLen: s19][>].var! #
    [0xXXXXXXXX (&ImplicitCallFlags)].u8 = MOV  1 (0x1).i8                    #
    [0xXXXXXXXX (&DisableImplicitCallFlags)].i8 = MOV  1 (0x1).i8             #
                       CALL           Op_SetElementI.u32                      #001d
    [0xXXXXXXXX (&DisableImplicitCallFlags)].i8 = MOV  0 (0x0).i8             #
                       CMP            [0xXXXXXXXX (&ImplicitCallFlags)].u8, 1 (0x1).i8 #
                       JEQ            $L17                                    #
$L18: [helper]                                                                #
    [0xXXXXXXXX (&BailOutKind)].u32 = MOV  5 (0x5).u32                        #
                       JMP            $L15                                    #
$L17: [helper]                                                                #
                       PUSH           s6[Array][seg: s18][segLen: s19][>].var #
                       PUSH           s22.u32                                 #
    s25(eax).u8     =  CALL           Array_Jit_OperationInvalidatedArrayLength.u32 #
    s24.u8          =  MOV            s25(eax).u8                             #
                       TEST           s24.u8, s24.u8                          #
                       JEQ            $L14                                    #
$L16: [helper]                                                                #
    [0xXXXXXXXX (&BailOutKind)].u32 = MOV  262144 (0x40000).u32               #
                       JMP            $L15                                    #
$L15: [helper]                                                                #
                       CALL           SaveAllRegistersAndBailOut.u32          #001d  Bailout: #001d (BailOutShared)
                       JMP            $L11                                    #
$L14: [helper]                                                                #
$L12:                                                                         #

Perf comparison (using pgo binaries)

Octane            Left score        Right score       ∆ Score  ∆ Score %  Comment
----------------  ----------------  ----------------  -------  ---------  --------
Box2d              45511.18 ±0.06%   45702.24 ±0.24%   191.05      0.42%
Code-load          21755.90 ±0.13%   21771.80 ±0.29%    15.90      0.07%
Crypto             34798.80 ±0.23%   34730.82 ±0.09%   -67.98     -0.20%
Deltablue          26570.30 ±0.35%   26334.31 ±0.30%  -235.99     -0.89%
Earley-boyer       41226.33 ±0.23%   40930.20 ±0.32%  -296.13     -0.72%
Gbemu              62391.77 ±0.06%   62760.10 ±0.11%   368.33      0.59%  Improved
Mandreel           34023.00 ±0.04%   34066.44 ±0.11%    43.44      0.13%
Mandreel latency  123778.80 ±0.23%  123447.78 ±0.29%  -331.02     -0.27%
Navier-stokes      36588.90 ±0.18%   36673.70 ±0.10%    84.80      0.23%
Pdfjs              25950.50 ±0.30%   26004.70 ±0.24%    54.20      0.21%
Raytrace           56779.20 ±0.25%   56604.90 ±0.83%  -174.30     -0.31%
Regexp              6161.00 ±0.47%    6135.27 ±0.37%   -25.73     -0.42%
Richards           29961.80 ±0.39%   30213.70 ±0.10%   251.90      0.84%
Splay              26685.27 ±0.33%   26783.53 ±0.29%    98.26      0.37%
Splay latency      46631.86 ±0.35%   46534.15 ±0.44%   -97.71     -0.21%
Typescript         31457.75 ±0.76%   31530.08 ±0.94%    72.33      0.23%
Zlib               67508.50 ±0.11%   67255.87 ±0.31%  -252.63     -0.37%
----------------  ----------------  ----------------  -------  ---------  --------
Total              35801.50 ±0.26%   35795.09 ±0.32%    -6.41     -0.02%

This change is Reviewable

@Cellule
Copy link
Contributor Author

Cellule commented Apr 13, 2016

@LouisLaf @pleath @rajatd Can one of you guys review this please ?

@pleath
Copy link
Contributor

pleath commented Apr 13, 2016

LGTM.

@chakrabot chakrabot merged commit 5b06b98 into chakra-core:release/1.2 Apr 13, 2016
chakrabot pushed a commit that referenced this pull request Apr 13, 2016
Merge pull request #802 from Cellule:array/invalid_length
The StElemI fast was ignoring the InvalidatedArrayLength bailout.
This change verifies that the array length assumption is still valid and go to helper if not
@Cellule Cellule deleted the array/invalid_length branch April 13, 2016 22:18
chakrabot pushed a commit that referenced this pull request Apr 13, 2016
…th bailout

Merge pull request #802 from Cellule:array/invalid_length
The StElemI fast was ignoring the InvalidatedArrayLength bailout.
This change verifies that the array length assumption is still valid and go to helper if not
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants