Bug 157668 – [JSC] Move the CheckTierUp function calls out of the main path

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[JSC] Move the CheckTierUp function calls out of the main path

[Benjamin Poulain]

Created attachment 278820 [details]
Patch

[Benjamin Poulain]

                                    Conf#1                    Conf#2                                      

3d-cube                         4.9750+-0.0183     ^      4.9216+-0.0172        ^ definitely 1.0109x faster
3d-morph                        5.0505+-0.0241     ?      5.0733+-0.0230        ?
3d-raytrace                     5.3486+-0.0268            5.3325+-0.0193        
access-binary-trees             2.1137+-0.0118     ?      2.1173+-0.0128        ?
access-fannkuch                 5.8436+-0.0222     !      5.8900+-0.0228        ! definitely 1.0079x slower
access-nbody                    2.5166+-0.0112            2.5166+-0.0113        
access-nsieve                   2.9611+-0.0119            2.9548+-0.0111        
bitops-3bit-bits-in-byte        1.1139+-0.0057     ?      1.1150+-0.0052        ?
bitops-bits-in-byte             2.7480+-0.0127     ^      2.5887+-0.0131        ^ definitely 1.0615x faster
bitops-bitwise-and              1.9873+-0.0075     ?      1.9899+-0.0084        ?
bitops-nsieve-bits              3.0469+-0.0113     !      3.0874+-0.0177        ! definitely 1.0133x slower
controlflow-recursive           2.3395+-0.0124            2.3363+-0.0104        
crypto-aes                      4.3709+-0.0236     ?      4.3782+-0.0252        ?
crypto-md5                      2.4169+-0.0105            2.4102+-0.0120        
crypto-sha1                     2.2562+-0.0137     ?      2.2636+-0.0145        ?
date-format-tofte               6.5461+-0.0253            6.5453+-0.0264        
date-format-xparb               4.8131+-0.0263            4.7939+-0.0233        
math-cordic                     2.8892+-0.0084     ^      2.8247+-0.0128        ^ definitely 1.0228x faster
math-partial-sums               3.7110+-0.0206     ?      3.7177+-0.0236        ?
math-spectral-norm              1.9969+-0.0120            1.9814+-0.0069        
regexp-dna                      6.5614+-0.0364            6.5493+-0.0349        
string-base64                   4.2191+-0.0257     ?      4.2558+-0.0311        ?
string-fasta                    5.6710+-0.0185            5.6655+-0.0172        
string-tagcloud                 8.5184+-0.0336     ?      8.5624+-0.0362        ?
string-unpack-code             18.8462+-0.0945     ?     18.9783+-0.0952        ?
string-validate-input           4.1754+-0.0199            4.1531+-0.0188        

<arithmetic>                    4.5014+-0.0053            4.5001+-0.0054          might be 1.0003x faster

[Mark Lam]

Comment on attachment 278820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278820&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5249
> -        m_jit.setupArgumentsWithExecState(TrustedImm32(bytecodeIndex));
> -        appendCallSetResult(triggerOSREntryNow, tempGPR);
> -        MacroAssembler::Jump dontEnter = m_jit.branchTestPtr(MacroAssembler::Zero, tempGPR);
> -        m_jit.emitRestoreCalleeSaves();
> -        m_jit.jump(tempGPR);
> -        dontEnter.link(&m_jit);
> -        silentFillAllRegisters(tempGPR);
> -        
> -        done.link(&m_jit);
> +
> +        addSlowPathGenerator([=]() {
> +            forceOSREntry.link(&m_jit);
> +            overflowedCounter.link(&m_jit);
> +
> +            silentSpill(savePlans);
> +            m_jit.setupArgumentsWithExecState(TrustedImm32(bytecodeIndex));
> +            appendCallSetResult(triggerOSREntryNow, tempGPR);
> +
> +            if (savePlans.isEmpty())
> +                m_jit.branchTestPtr(MacroAssembler::Zero, tempGPR).linkTo(toNextOperation, &m_jit);
> +            else {
> +                MacroAssembler::Jump osrEnter = m_jit.branchTestPtr(MacroAssembler::NonZero, tempGPR);
> +                silentFill(savePlans);
> +                osrEnter.link(&m_jit);
> +            }
> +            m_jit.emitRestoreCalleeSaves();
> +            m_jit.jump(tempGPR);
> +        });

This does not look right.  In the old code, of tempGPR is Zero, we do not enter.  In the new code, I don't see a path to not enter.  I think you're missing a jump to toNextOperation after the silentFill.

[Benjamin Poulain]

Created attachment 278846 [details]
Patch

[Mark Lam]

Comment on attachment 278846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278846&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5246
> +                MacroAssembler::Jump osrEnter = m_jit.branchTestPtr(MacroAssembler::NonZero, tempGPR);
> +                silentFill(savePlans);
> +                osrEnter.link(&m_jit);
> +                m_jit.jump().linkTo(toNextOperation, &m_jit);

Still wrong.  The jump needs to be before the osrEnter label linkage.

[Benjamin Poulain]

Created attachment 278856 [details]
Patch

[Mark Lam]

Comment on attachment 278856 [details]
Patch

r=me

[Benjamin Poulain]

Comment on attachment 278856 [details]
Patch

Clearing flags on attachment: 278856

Committed r200897: <http://trac.webkit.org/changeset/200897>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.