Changeset 149152 in webkit


Ignore:
Timestamp:
Apr 25, 2013 3:52:34 PM (11 years ago)
Author:
commit-queue@webkit.org
Message:

Source/JavaScriptCore: Fix problems with processing negative zero on DFG.
​https://bugs.webkit.org/show_bug.cgi?id=113862

Patch by Roman Zhuykov <​zhroma@ispras.ru> on 2013-04-25
Reviewed by Filip Pizlo.

Fix NodeNeedsNegZero flag propagation in BackwardPropagationPhase.
Function arithNodeFlags should not mask NodeNeedsNegZero flag for ArithNegate and DoubleAsInt32
nodes and this flag should be always used to decide where we need to generate nezative-zero checks.
Remove unnecessary negative-zero checks from integer ArithDiv on ARM.
Also remove such checks from integer ArithMod on ARM and X86, and make them always to
check not only "modulo_result == 0" but also "dividend < 0".
Generate faster code for case when ArithMod operation divisor is constant power of 2 on ARMv7
in the same way as on ARMv7s, and add negative-zero checks into this code when needed.
Change speculationCheck ExitKind from Overflow to NegativeZero where applicable.

This shows 30% speedup of math-spectral-norm, and 5% speedup
on SunSpider overall on ARMv7 Linux.

  • assembler/MacroAssemblerARM.h:

(JSC::MacroAssemblerARM::branchConvertDoubleToInt32):

  • assembler/MacroAssemblerARMv7.h:

(JSC::MacroAssemblerARMv7::branchConvertDoubleToInt32):

  • assembler/MacroAssemblerMIPS.h:

(JSC::MacroAssemblerMIPS::branchConvertDoubleToInt32):

  • assembler/MacroAssemblerSH4.h:

(JSC::MacroAssemblerSH4::branchConvertDoubleToInt32):

  • assembler/MacroAssemblerX86Common.h:

(JSC::MacroAssemblerX86Common::branchConvertDoubleToInt32):

  • dfg/DFGBackwardsPropagationPhase.cpp:

(JSC::DFG::BackwardsPropagationPhase::isNotNegZero):
(JSC::DFG::BackwardsPropagationPhase::isNotPosZero):
(JSC::DFG::BackwardsPropagationPhase::propagate):

  • dfg/DFGNode.h:

(JSC::DFG::Node::arithNodeFlags):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileDoubleAsInt32):
(JSC::DFG::SpeculativeJIT::compileSoftModulo):
(JSC::DFG::SpeculativeJIT::compileArithNegate):

LayoutTests: Arithmetic operations with negative zero should be optimized correclty.
​https://bugs.webkit.org/show_bug.cgi?id=113862

Patch by Roman Zhuykov <​zhroma@ispras.ru> on 2013-04-25
Reviewed by Filip Pizlo.

  • fast/js/regress/negative-zero-divide-expected.txt: Added.
  • fast/js/regress/negative-zero-divide.html: Added.
  • fast/js/regress/negative-zero-modulo-expected.txt: Added.
  • fast/js/regress/negative-zero-modulo.html: Added.
  • fast/js/regress/negative-zero-negate-expected.txt: Added.
  • fast/js/regress/negative-zero-negate.html: Added.
  • fast/js/regress/script-tests/negative-zero-divide.js: Added.

(foo):

  • fast/js/regress/script-tests/negative-zero-modulo.js: Added.

(foo):

  • fast/js/regress/script-tests/negative-zero-negate.js: Added.

(foo):

Location:
trunk
Files:
9 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r149151 r149152  
     12013-04-25  Roman Zhuykov  <zhroma@ispras.ru>
     2
     3        Arithmetic operations with negative zero should be optimized correclty.
     4        https://bugs.webkit.org/show_bug.cgi?id=113862
     5
     6        Reviewed by Filip Pizlo.
     7        * fast/js/regress/negative-zero-divide-expected.txt: Added.
     8        * fast/js/regress/negative-zero-divide.html: Added.
     9        * fast/js/regress/negative-zero-modulo-expected.txt: Added.
     10        * fast/js/regress/negative-zero-modulo.html: Added.
     11        * fast/js/regress/negative-zero-negate-expected.txt: Added.
     12        * fast/js/regress/negative-zero-negate.html: Added.
     13        * fast/js/regress/script-tests/negative-zero-divide.js: Added.
     14        (foo):
     15        * fast/js/regress/script-tests/negative-zero-modulo.js: Added.
     16        (foo):
     17        * fast/js/regress/script-tests/negative-zero-negate.js: Added.
     18        (foo):
     19
    1202013-04-25  Kent Tamura  <tkent@chromium.org>
    221

  • trunk/Source/JavaScriptCore/ChangeLog

    r149146 r149152  
     12013-04-25  Roman Zhuykov  <zhroma@ispras.ru>
     2
     3        Fix problems with processing negative zero on DFG.
     4        https://bugs.webkit.org/show_bug.cgi?id=113862
     5
     6        Reviewed by Filip Pizlo.
     7
     8        Fix NodeNeedsNegZero flag propagation in BackwardPropagationPhase.
     9        Function arithNodeFlags should not mask NodeNeedsNegZero flag for ArithNegate and DoubleAsInt32
     10        nodes and this flag should be always used to decide where we need to generate nezative-zero checks.
     11        Remove unnecessary negative-zero checks from integer ArithDiv on ARM.
     12        Also remove such checks from integer ArithMod on ARM and X86, and make them always to
     13        check not only "modulo_result == 0" but also "dividend < 0".
     14        Generate faster code for case when ArithMod operation divisor is constant power of 2 on ARMv7
     15        in the same way as on ARMv7s, and add negative-zero checks into this code when needed.
     16        Change speculationCheck ExitKind from Overflow to NegativeZero where applicable.
     17 
     18        This shows 30% speedup of math-spectral-norm, and 5% speedup
     19        on SunSpider overall on ARMv7 Linux.
     20
     21        * assembler/MacroAssemblerARM.h:
     22        (JSC::MacroAssemblerARM::branchConvertDoubleToInt32):
     23        * assembler/MacroAssemblerARMv7.h:
     24        (JSC::MacroAssemblerARMv7::branchConvertDoubleToInt32):
     25        * assembler/MacroAssemblerMIPS.h:
     26        (JSC::MacroAssemblerMIPS::branchConvertDoubleToInt32):
     27        * assembler/MacroAssemblerSH4.h:
     28        (JSC::MacroAssemblerSH4::branchConvertDoubleToInt32):
     29        * assembler/MacroAssemblerX86Common.h:
     30        (JSC::MacroAssemblerX86Common::branchConvertDoubleToInt32):
     31        * dfg/DFGBackwardsPropagationPhase.cpp:
     32        (JSC::DFG::BackwardsPropagationPhase::isNotNegZero):
     33        (JSC::DFG::BackwardsPropagationPhase::isNotPosZero):
     34        (JSC::DFG::BackwardsPropagationPhase::propagate):
     35        * dfg/DFGNode.h:
     36        (JSC::DFG::Node::arithNodeFlags):
     37        * dfg/DFGSpeculativeJIT.cpp:
     38        (JSC::DFG::SpeculativeJIT::compileDoubleAsInt32):
     39        (JSC::DFG::SpeculativeJIT::compileSoftModulo):
     40        (JSC::DFG::SpeculativeJIT::compileArithNegate):
     41
    1422013-04-25  Oliver Hunt  <oliver@apple.com>
    243

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM.h

    r148893 r149152  
    12451245    // May also branch for some values that are representable in 32 bits
    12461246    // (specifically, in this case, 0).
    1247     void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID)
     1247    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID, bool negZeroCheck = true)
    12481248    {
    12491249        m_assembler.vcvt_s32_f64(ARMRegisters::SD0 << 1, src);
    … …  
    12551255
    12561256        // If the result is zero, it might have been -0.0, and 0.0 equals to -0.0
    1257         failureCases.append(branchTest32(Zero, dest));
     1257        if (negZeroCheck)
     1258            failureCases.append(branchTest32(Zero, dest));
    12581259    }
    12591260

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

    r148942 r149152  
    11051105    // May also branch for some values that are representable in 32 bits
    11061106    // (specifically, in this case, 0).
    1107     void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID)
     1107    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID, bool negZeroCheck = true)
    11081108    {
    11091109        m_assembler.vcvt_floatingPointToSigned(fpTempRegisterAsSingle(), src);
    … …  
    11151115
    11161116        // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
    1117         failureCases.append(branchTest32(Zero, dest));
     1117        if (negZeroCheck)
     1118            failureCases.append(branchTest32(Zero, dest));
    11181119    }
    11191120

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h

    r148893 r149152  
    26302630    // May also branch for some values that are representable in 32 bits
    26312631    // (specifically, in this case, 0).
    2632     void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
     2632    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
    26332633    {
    26342634        m_assembler.cvtwd(fpTempRegister, src);
    … …  
    26362636
    26372637        // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
    2638         failureCases.append(branch32(Equal, dest, MIPSRegisters::zero));
     2638        if (negZeroCheck)
     2639            failureCases.append(branch32(Equal, dest, MIPSRegisters::zero));
    26392640
    26402641        // Convert the integer result back to float & compare to the original value - if not equal or unordered (NaN) then jump.

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerSH4.h

    r149078 r149152  
    20372037    }
    20382038
    2039     void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
     2039    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
    20402040    {
    20412041        m_assembler.ftrcdrmfpul(src);
    … …  
    20442044        failureCases.append(branchDouble(DoubleNotEqualOrUnordered, fscratch, src));
    20452045
    2046         if (dest == SH4Registers::r0)
    2047             m_assembler.cmpEqImmR0(0, dest);
    2048         else {
    2049             m_assembler.movImm8(0, scratchReg3);
    2050             m_assembler.cmplRegReg(scratchReg3, dest, SH4Condition(Equal));
    2051         }
    2052         failureCases.append(branchTrue());
     2046        if (negZeroCheck) {
     2047            if (dest == SH4Registers::r0)
     2048                m_assembler.cmpEqImmR0(0, dest);
     2049            else {
     2050                m_assembler.movImm8(0, scratchReg3);
     2051                m_assembler.cmplRegReg(scratchReg3, dest, SH4Condition(Equal));
     2052            }
     2053            failureCases.append(branchTrue());
     2054        }
    20532055    }
    20542056

  • trunk/Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h

    r148893 r149152  
    886886    // May also branch for some values that are representable in 32 bits
    887887    // (specifically, in this case, 0).
    888     void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp)
     888    void branchConvertDoubleToInt32(FPRegisterID src, RegisterID dest, JumpList& failureCases, FPRegisterID fpTemp, bool negZeroCheck = true)
    889889    {
    890890        ASSERT(isSSE2Present());
    … …  
    892892
    893893        // If the result is zero, it might have been -0.0, and the double comparison won't catch this!
    894         failureCases.append(branchTest32(Zero, dest));
     894        if (negZeroCheck)
     895            failureCases.append(branchTest32(Zero, dest));
    895896
    896897        // Convert the integer result back to float & compare to the original value - if not equal or unordered (NaN) then jump.

  • trunk/Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp

    r146382 r149152  
    6767            return false;
    6868        double value = m_graph.valueOfNumberConstant(node);
    69         return !value && 1.0 / value < 0.0;
    70     }
    71    
    72     bool isNotZero(Node* node)
     69        return (value || 1.0 / value > 0.0);
     70    }
     71   
     72    bool isNotPosZero(Node* node)
    7373    {
    7474        if (!m_graph.isNumberConstant(node))
    7575            return false;
    76         return !!m_graph.valueOfNumberConstant(node);
     76        double value = m_graph.valueOfNumberConstant(node);
     77        return (value || 1.0 / value < 0.0);
    7778    }
    7879
    … …  
    248249           
    249250        case ArithSub: {
    250             if (isNotZero(node->child1().node()) || isNotZero(node->child2().node()))
     251            if (isNotNegZero(node->child1().node()) || isNotPosZero(node->child2().node()))
    251252                flags &= ~NodeNeedsNegZero;
    252253            if (!isWithinPowerOfTwo<32>(node->child1()) && !isWithinPowerOfTwo<32>(node->child2()))

  • trunk/Source/JavaScriptCore/dfg/DFGNode.h

    r149041 r149152  
    523523    {
    524524        NodeFlags result = m_flags & NodeArithFlagsMask;
    525         if (op() == ArithMul || op() == ArithDiv || op() == ArithMod)
     525        if (op() == ArithMul || op() == ArithDiv || op() == ArithMod || op() == ArithNegate || op() == DoubleAsInt32)
    526526            return result;
    527527        return result & ~NodeNeedsNegZero;

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

    r149050 r149152  
    23962396
    23972397    JITCompiler::JumpList failureCases;
    2398     m_jit.branchConvertDoubleToInt32(valueFPR, resultGPR, failureCases, scratchFPR);
     2398    bool negZeroCheck = !nodeCanIgnoreNegativeZero(node->arithNodeFlags());
     2399    m_jit.branchConvertDoubleToInt32(valueFPR, resultGPR, failureCases, scratchFPR, negZeroCheck);
    23992400    forwardSpeculationCheck(Overflow, JSValueRegs(), 0, failureCases, ValueRecovery::inFPR(valueFPR));
    24002401
    … …  
    29092910            m_jit.assembler().cdq();
    29102911            m_jit.assembler().idivl_r(scratchGPR);
    2911             // Check that we're not about to create negative zero.
    2912             // FIXME: if the node use doesn't care about neg zero, we can do this more easily.
    2913             JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
    2914             speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
    2915             numeratorPositive.link(&m_jit);
    2916            
     2912            if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags())) {
     2913                // Check that we're not about to create negative zero.
     2914                JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
     2915                speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
     2916                numeratorPositive.link(&m_jit);
     2917            }
    29172918            if (op1SaveGPR != op1Gpr)
    29182919                unlock(op1SaveGPR);
    … …  
    29222923        }
    29232924    }
    2924 #elif CPU(APPLE_ARMV7S)
     2925#elif CPU(APPLE_ARMV7S) || CPU(ARM_THUMB2)
    29252926    if (isInt32Constant(node->child2().node())) {
    29262927        int32_t divisor = valueOfInt32Constant(node->child2().node());
    … …  
    29382939            m_jit.assembler().neg(resultGPR, resultGPR);
    29392940
     2941            if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags())) {
     2942                // Check that we're not about to create negative zero.
     2943                JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, dividendGPR, TrustedImm32(0));
     2944                speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, resultGPR));
     2945                numeratorPositive.link(&m_jit);
     2946            }
    29402947            integerResult(resultGPR, node);
    29412948            return;
    … …  
    30013008        unlock(op2TempGPR);
    30023009
    3003     // Check that we're not about to create negative zero.
    3004     // FIXME: if the node use doesn't care about neg zero, we can do this more easily.
    3005     JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
    3006     speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
    3007     numeratorPositive.link(&m_jit);
     3010    if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags())) {
     3011        // Check that we're not about to create negative zero.
     3012        JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1SaveGPR, TrustedImm32(0));
     3013        speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, edx.gpr()));
     3014        numeratorPositive.link(&m_jit);
     3015    }
    30083016   
    30093017    if (op1SaveGPR != op1GPR)
    … …  
    30303038        // Check that we're not about to create negative zero.
    30313039        JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, dividendGPR, TrustedImm32(0));
    3032         speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, quotientThenRemainderGPR));
     3040        speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, quotientThenRemainderGPR));
    30333041        numeratorPositive.link(&m_jit);
    30343042    }
    … …  
    30493057    GPRTemporary intResult(this);
    30503058    JITCompiler::JumpList failureCases;
    3051     m_jit.branchConvertDoubleToInt32(result.fpr(), intResult.gpr(), failureCases, scratch.fpr());
     3059    m_jit.branchConvertDoubleToInt32(result.fpr(), intResult.gpr(), failureCases, scratch.fpr(), false);
    30523060    speculationCheck(Overflow, JSValueRegs(), 0, failureCases);
     3061    if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags())) {
     3062        // Check that we're not about to create negative zero.
     3063        JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1GPR, TrustedImm32(0));
     3064        speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, intResult.gpr()));
     3065        numeratorPositive.link(&m_jit);
     3066    }
    30533067   
    30543068    integerResult(intResult.gpr(), node);
    … …  
    32973311            speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchNeg32(MacroAssembler::Overflow, result.gpr()));
    32983312            if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags()))
    3299                 speculationCheck(Overflow, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, result.gpr()));
     3313                speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, result.gpr()));
    33003314        }
    33013315
Note: See TracChangeset for help on using the changeset viewer.