⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 243959 in webkit


Ignore:
Timestamp:
Apr 5, 2019, 6:57:16 PM (7 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern
​https://bugs.webkit.org/show_bug.cgi?id=196582

Reviewed by Saam Barati.

JSTests:

  • stress/add-overflow-check-with-three-same-registers.js: Added.

(foo):
(Number.prototype.valueOf):
(runWithNumber):

Source/JavaScriptCore:

In DFG, our ArithAdd with overflow is executed speculatively, and we recover the value when overflow flag is set.
The recovery is subtracting the operand from the destination to get the original two operands. Our recovery code
handles A + B = A, A + B = B cases. But it misses A + A = A case (here, A and B are GPRReg). Our recovery code
attempts to produce the original operand by performing A - A, and it always produces zero accidentally.

This patch adds the recovery code for A + A = A case. Because we know that this ArithAdd overflows, and operands were
same values, we can calculate the original operand from the destination value by ((int32_t)value >> 1) ^ 0x80000000.

We also found that FTL recovery code is dead. We remove them in this patch.

  • dfg/DFGOSRExit.cpp:

(JSC::DFG::OSRExit::executeOSRExit):
(JSC::DFG::OSRExit::compileExit):

  • dfg/DFGOSRExit.h:

(JSC::DFG::SpeculationRecovery::SpeculationRecovery):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileArithAdd):

  • ftl/FTLExitValue.cpp:

(JSC::FTL::ExitValue::dataFormat const):
(JSC::FTL::ExitValue::dumpInContext const):

  • ftl/FTLExitValue.h:

(JSC::FTL::ExitValue::isArgument const):
(JSC::FTL::ExitValue::hasIndexInStackmapLocations const):
(JSC::FTL::ExitValue::adjustStackmapLocationsIndexByOffset):
(JSC::FTL::ExitValue::recovery): Deleted.
(JSC::FTL::ExitValue::isRecovery const): Deleted.
(JSC::FTL::ExitValue::leftRecoveryArgument const): Deleted.
(JSC::FTL::ExitValue::rightRecoveryArgument const): Deleted.
(JSC::FTL::ExitValue::recoveryFormat const): Deleted.
(JSC::FTL::ExitValue::recoveryOpcode const): Deleted.

  • ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::preparePatchpointForExceptions):
(JSC::FTL::DFG::LowerDFGToB3::appendOSRExit):
(JSC::FTL::DFG::LowerDFGToB3::exitValueForNode):
(JSC::FTL::DFG::LowerDFGToB3::addAvailableRecovery): Deleted.

  • ftl/FTLOSRExitCompiler.cpp:

(JSC::FTL::compileRecovery):

Location:
trunk
Files:
1 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r243955 r243959  
     12019-04-05  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern
     4        https://bugs.webkit.org/show_bug.cgi?id=196582
     5
     6        Reviewed by Saam Barati.
     7
     8        * stress/add-overflow-check-with-three-same-registers.js: Added.
     9        (foo):
     10        (Number.prototype.valueOf):
     11        (runWithNumber):
     12
    1132019-04-05  Ryan Haddad  <ryanhaddad@apple.com>
    214

  • trunk/Source/JavaScriptCore/ChangeLog

    r243955 r243959  
     12019-04-05  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] OSRExit recovery for SpeculativeAdd does not consier "A = A + A" pattern
     4        https://bugs.webkit.org/show_bug.cgi?id=196582
     5
     6        Reviewed by Saam Barati.
     7
     8        In DFG, our ArithAdd with overflow is executed speculatively, and we recover the value when overflow flag is set.
     9        The recovery is subtracting the operand from the destination to get the original two operands. Our recovery code
     10        handles A + B = A, A + B = B cases. But it misses A + A = A case (here, A and B are GPRReg). Our recovery code
     11        attempts to produce the original operand by performing A - A, and it always produces zero accidentally.
     12
     13        This patch adds the recovery code for A + A = A case. Because we know that this ArithAdd overflows, and operands were
     14        same values, we can calculate the original operand from the destination value by `((int32_t)value >> 1) ^ 0x80000000`.
     15
     16        We also found that FTL recovery code is dead. We remove them in this patch.
     17
     18        * dfg/DFGOSRExit.cpp:
     19        (JSC::DFG::OSRExit::executeOSRExit):
     20        (JSC::DFG::OSRExit::compileExit):
     21        * dfg/DFGOSRExit.h:
     22        (JSC::DFG::SpeculationRecovery::SpeculationRecovery):
     23        * dfg/DFGSpeculativeJIT.cpp:
     24        (JSC::DFG::SpeculativeJIT::compileArithAdd):
     25        * ftl/FTLExitValue.cpp:
     26        (JSC::FTL::ExitValue::dataFormat const):
     27        (JSC::FTL::ExitValue::dumpInContext const):
     28        * ftl/FTLExitValue.h:
     29        (JSC::FTL::ExitValue::isArgument const):
     30        (JSC::FTL::ExitValue::hasIndexInStackmapLocations const):
     31        (JSC::FTL::ExitValue::adjustStackmapLocationsIndexByOffset):
     32        (JSC::FTL::ExitValue::recovery): Deleted.
     33        (JSC::FTL::ExitValue::isRecovery const): Deleted.
     34        (JSC::FTL::ExitValue::leftRecoveryArgument const): Deleted.
     35        (JSC::FTL::ExitValue::rightRecoveryArgument const): Deleted.
     36        (JSC::FTL::ExitValue::recoveryFormat const): Deleted.
     37        (JSC::FTL::ExitValue::recoveryOpcode const): Deleted.
     38        * ftl/FTLLowerDFGToB3.cpp:
     39        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
     40        (JSC::FTL::DFG::LowerDFGToB3::preparePatchpointForExceptions):
     41        (JSC::FTL::DFG::LowerDFGToB3::appendOSRExit):
     42        (JSC::FTL::DFG::LowerDFGToB3::exitValueForNode):
     43        (JSC::FTL::DFG::LowerDFGToB3::addAvailableRecovery): Deleted.
     44        * ftl/FTLOSRExitCompiler.cpp:
     45        (JSC::FTL::compileRecovery):
     46
    1472019-04-05  Ryan Haddad  <ryanhaddad@apple.com>
    248

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

    r243286 r243959  
    470470            case SpeculativeAdd:
    471471                cpu.gpr(recovery->dest()) = cpu.gpr<uint32_t>(recovery->dest()) - cpu.gpr<uint32_t>(recovery->src());
     472#if USE(JSVALUE64)
     473                ASSERT(!(cpu.gpr(recovery->dest()) >> 32));
     474                cpu.gpr(recovery->dest()) |= TagTypeNumber;
     475#endif
     476                break;
     477
     478            case SpeculativeAddSelf:
     479                cpu.gpr(recovery->dest()) = static_cast<uint32_t>(cpu.gpr<int32_t>(recovery->dest()) >> 1) ^ 0x80000000U;
    472480#if USE(JSVALUE64)
    473481                ASSERT(!(cpu.gpr(recovery->dest()) >> 32));
    … …  
    11181126            break;
    11191127
     1128        case SpeculativeAddSelf:
     1129            // If A + A = A (int32_t) overflows, A can be recovered by ((static_cast<int32_t>(A) >> 1) ^ 0x8000000).
     1130            jit.rshift32(AssemblyHelpers::TrustedImm32(1), recovery->dest());
     1131            jit.xor32(AssemblyHelpers::TrustedImm32(0x80000000), recovery->dest());
     1132#if USE(JSVALUE64)
     1133            jit.or64(GPRInfo::tagTypeNumberRegister, recovery->dest());
     1134#endif
     1135            break;
     1136
    11201137        case SpeculativeAddImmediate:
    11211138            jit.sub32(AssemblyHelpers::Imm32(recovery->immediate()), recovery->dest());

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

    r236585 r243959  
    6060enum SpeculationRecoveryType : uint8_t {
    6161    SpeculativeAdd,
     62    SpeculativeAddSelf,
    6263    SpeculativeAddImmediate,
    6364    BooleanSpeculationCheck
    … …  
    7576        , m_type(type)
    7677    {
    77         ASSERT(m_type == SpeculativeAdd || m_type == BooleanSpeculationCheck);
     78        ASSERT(m_type == SpeculativeAdd || m_type == SpeculativeAddSelf || m_type == BooleanSpeculationCheck);
    7879    }
    7980

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

    r243364 r243959  
    43074307            MacroAssembler::Jump check = m_jit.branchAdd32(MacroAssembler::Overflow, gpr1, gpr2, gprResult);
    43084308               
    4309             if (gpr1 == gprResult)
     4309            if (gpr1 == gprResult && gpr2 == gprResult)
     4310                speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAddSelf, gprResult, gpr2));
     4311            else if (gpr1 == gprResult)
    43104312                speculationCheck(Overflow, JSValueRegs(), 0, check, SpeculationRecovery(SpeculativeAdd, gprResult, gpr2));
    43114313            else if (gpr2 == gprResult)

  • trunk/Source/JavaScriptCore/ftl/FTLExitValue.cpp

    r189362 r243959  
    7676    case ExitValueInJSStackAsDouble:
    7777        return DataFormatDouble;
    78            
    79     case ExitValueRecovery:
    80         return recoveryFormat();
    8178    }
    8279       
    … …  
    111108        out.print("InJSStackAsDouble:", virtualRegister());
    112109        return;
    113     case ExitValueRecovery:
    114         out.print("Recovery(", recoveryOpcode(), ", arg", leftRecoveryArgument(), ", arg", rightRecoveryArgument(), ", ", recoveryFormat(), ")");
    115         return;
    116110    case ExitValueMaterializeNewObject:
    117111        out.print("Materialize(", WTF::RawPointer(objectMaterialization()), ")");

  • trunk/Source/JavaScriptCore/ftl/FTLExitValue.h

    r206525 r243959  
    5555    ExitValueInJSStackAsInt52,
    5656    ExitValueInJSStackAsDouble,
    57     ExitValueRecovery,
    5857    ExitValueMaterializeNewObject
    5958};
    … …  
    122121        result.m_kind = ExitValueArgument;
    123122        result.u.argument = argument.representation();
    124         return result;
    125     }
    126    
    127     static ExitValue recovery(RecoveryOpcode opcode, unsigned leftArgument, unsigned rightArgument, DataFormat format)
    128     {
    129         ExitValue result;
    130         result.m_kind = ExitValueRecovery;
    131         result.u.recovery.opcode = opcode;
    132         result.u.recovery.leftArgument = leftArgument;
    133         result.u.recovery.rightArgument = rightArgument;
    134         result.u.recovery.format = format;
    135123        return result;
    136124    }
    … …  
    155143    bool isConstant() const { return kind() == ExitValueConstant; }
    156144    bool isArgument() const { return kind() == ExitValueArgument; }
    157     bool isRecovery() const { return kind() == ExitValueRecovery; }
    158145    bool isObjectMaterialization() const { return kind() == ExitValueMaterializeNewObject; }
    159     bool hasIndexInStackmapLocations() const { return isArgument() || isRecovery(); }
     146    bool hasIndexInStackmapLocations() const { return isArgument(); }
    160147   
    161148    ExitArgument exitArgument() const
    … …  
    165152    }
    166153   
    167     unsigned leftRecoveryArgument() const
    168     {
    169         ASSERT(isRecovery());
    170         return u.recovery.leftArgument;
    171     }
    172    
    173     unsigned rightRecoveryArgument() const
    174     {
    175         ASSERT(isRecovery());
    176         return u.recovery.rightArgument;
    177     }
    178 
    179154    void adjustStackmapLocationsIndexByOffset(unsigned offset)
    180155    {
    181156        ASSERT(hasIndexInStackmapLocations());
    182         if (isArgument())
    183             u.argument.argument += offset;
    184         else {
    185             ASSERT(isRecovery());
    186             u.recovery.rightArgument += offset;
    187             u.recovery.leftArgument += offset;
    188         }
    189     }
    190    
    191     DataFormat recoveryFormat() const
    192     {
    193         ASSERT(isRecovery());
    194         return static_cast<DataFormat>(u.recovery.format);
    195     }
    196    
    197     RecoveryOpcode recoveryOpcode() const
    198     {
    199         ASSERT(isRecovery());
    200         return static_cast<RecoveryOpcode>(u.recovery.opcode);
     157        ASSERT(isArgument());
     158        u.argument.argument += offset;
    201159    }
    202160   
    … …  
    247205        EncodedJSValue constant;
    248206        int virtualRegister;
    249         struct {
    250             uint16_t leftArgument;
    251             uint16_t rightArgument;
    252             uint16_t opcode;
    253             uint16_t format;
    254         } recovery;
    255207        ExitTimeObjectMaterialization* newObjectMaterializationData;
    256208    } u;

  • trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

    r243596 r243959  
    676676        if (verboseCompilationEnabled())
    677677            dataLog("Lowering ", m_node, "\n");
    678        
    679         m_availableRecoveries.shrink(0);
    680678       
    681679        m_interpreter.startExecuting();
    … …  
    1674516743            return PatchpointExceptionHandle::defaultHandle(m_ftlState);
    1674616744
    16747         if (verboseCompilationEnabled()) {
    16748             dataLog("    Patchpoint exception OSR exit #", m_ftlState.jitCode->osrExitDescriptors.size(), " with availability: ", availabilityMap(), "\n");
    16749             if (!m_availableRecoveries.isEmpty())
    16750                 dataLog("        Available recoveries: ", listDump(m_availableRecoveries), "\n");
    16751         }
     16745        dataLogLnIf(verboseCompilationEnabled(), "    Patchpoint exception OSR exit #", m_ftlState.jitCode->osrExitDescriptors.size(), " with availability: ", availabilityMap());
    1675216746
    1675316747        bool exitOK = true;
    … …  
    1680316797        NodeOrigin origin, bool isExceptionHandler = false)
    1680416798    {
    16805         if (verboseCompilationEnabled()) {
    16806             dataLog("    OSR exit #", m_ftlState.jitCode->osrExitDescriptors.size(), " with availability: ", availabilityMap(), "\n");
    16807             if (!m_availableRecoveries.isEmpty())
    16808                 dataLog("        Available recoveries: ", listDump(m_availableRecoveries), "\n");
    16809         }
     16799        dataLogLnIf(verboseCompilationEnabled(), "    OSR exit #", m_ftlState.jitCode->osrExitDescriptors.size(), " with availability: ", availabilityMap());
    1681016800
    1681116801        DFG_ASSERT(m_graph, m_node, origin.exitOK);
    … …  
    1700116991        }
    1700216992       
    17003         for (unsigned i = 0; i < m_availableRecoveries.size(); ++i) {
    17004             AvailableRecovery recovery = m_availableRecoveries[i];
    17005             if (recovery.node() != node)
    17006                 continue;
    17007             ExitValue result = ExitValue::recovery(
    17008                 recovery.opcode(), arguments.size(), arguments.size() + 1,
    17009                 recovery.format());
    17010             arguments.append(recovery.left());
    17011             arguments.append(recovery.right());
    17012             return result;
    17013         }
    17014        
    1701516993        LoweredNodeValue value = m_int32Values.get(node);
    1701616994        if (isValid(value))
    … …  
    1707917057    }
    1708017058
    17081     void addAvailableRecovery(
    17082         Node* node, RecoveryOpcode opcode, LValue left, LValue right, DataFormat format)
    17083     {
    17084         m_availableRecoveries.append(AvailableRecovery(node, opcode, left, right, format));
    17085     }
    17086    
    17087     void addAvailableRecovery(
    17088         Edge edge, RecoveryOpcode opcode, LValue left, LValue right, DataFormat format)
    17089     {
    17090         addAvailableRecovery(edge.node(), opcode, left, right, format);
    17091     }
    17092    
    1709317059    void setInt32(Node* node, LValue value)
    1709417060    {
    … …  
    1735117317    LocalOSRAvailabilityCalculator m_availabilityCalculator;
    1735217318   
    17353     Vector<AvailableRecovery, 3> m_availableRecoveries;
    17354    
    1735517319    InPlaceAbstractState m_state;
    1735617320    AbstractInterpreter<InPlaceAbstractState> m_interpreter;

  • trunk/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp

    r243232 r243959  
    125125        break;
    126126           
    127     case ExitValueRecovery:
    128         Location::forValueRep(valueReps[value.rightRecoveryArgument()]).restoreInto(
    129             jit, registerScratch, GPRInfo::regT1);
    130         Location::forValueRep(valueReps[value.leftRecoveryArgument()]).restoreInto(
    131             jit, registerScratch, GPRInfo::regT0);
    132         switch (value.recoveryOpcode()) {
    133         case AddRecovery:
    134             switch (value.recoveryFormat()) {
    135             case DataFormatInt32:
    136                 jit.add32(GPRInfo::regT1, GPRInfo::regT0);
    137                 break;
    138             case DataFormatInt52:
    139                 jit.add64(GPRInfo::regT1, GPRInfo::regT0);
    140                 break;
    141             default:
    142                 RELEASE_ASSERT_NOT_REACHED();
    143                 break;
    144             }
    145             break;
    146         case SubRecovery:
    147             switch (value.recoveryFormat()) {
    148             case DataFormatInt32:
    149                 jit.sub32(GPRInfo::regT1, GPRInfo::regT0);
    150                 break;
    151             case DataFormatInt52:
    152                 jit.sub64(GPRInfo::regT1, GPRInfo::regT0);
    153                 break;
    154             default:
    155                 RELEASE_ASSERT_NOT_REACHED();
    156                 break;
    157             }
    158             break;
    159         default:
    160             RELEASE_ASSERT_NOT_REACHED();
    161             break;
    162         }
    163         break;
    164        
    165127    case ExitValueMaterializeNewObject:
    166128        jit.loadPtr(materializationToPointer.get(value.objectMaterialization()), GPRInfo::regT0);
Note: See TracChangeset for help on using the changeset viewer.