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

Changeset 280507 in webkit


Ignore:
Timestamp:
Jul 30, 2021, 6:40:05 PM (5 years ago)
Author:
rmorisset@apple.com
Message:

Improve OSR entry into Wasm loops with arguments
​https://bugs.webkit.org/show_bug.cgi?id=228595

Reviewed by Yusuke Suzuki.

JSTests:

Just a straightforward test that counts to 1M in a loop, to exercise both OSR entry and a loop with an argument at the same time.
100k iterations was not enough to reliably complete an OSR entry.

  • wasm/stress/osr-entry-with-loop-arguments.js: Added.

(async test):

Source/JavaScriptCore:

This patch has two parts:

  • improve the Wasm OSR code to fully support loop arguments (just some plumbing to make sure that the right values are propagated)
  • improve the B3 validator to fix a hole I noticed while writing the first part: we were not detecting code that introduce Upsilons in the wrong blocks. Naturally, this caused hard to debug issues, as B3 has no well-defined semantics for a Phi that is reached before the corresponding Upsilon(s).
  • b3/B3Validate.cpp:
  • wasm/WasmAirIRGenerator.cpp:

(JSC::Wasm::AirIRGenerator::emitLoopTierUpCheck):
(JSC::Wasm::AirIRGenerator::addLoop):

  • wasm/WasmB3IRGenerator.cpp:

(JSC::Wasm::B3IRGenerator::emitLoopTierUpCheck):
(JSC::Wasm::B3IRGenerator::addLoop):

  • wasm/WasmLLIntGenerator.cpp:

(JSC::Wasm::LLIntGenerator::addLoop):

Location:
trunk
Files:
1 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r280505 r280507  
     12021-07-30  Robin Morisset  <rmorisset@apple.com>
     2
     3        Improve OSR entry into Wasm loops with arguments
     4        https://bugs.webkit.org/show_bug.cgi?id=228595
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        Just a straightforward test that counts to 1M in a loop, to exercise both OSR entry and a loop with an argument at the same time.
     9        100k iterations was not enough to reliably complete an OSR entry.
     10
     11        * wasm/stress/osr-entry-with-loop-arguments.js: Added.
     12        (async test):
     13
    1142021-07-30  Tadeu Zagallo  <tzagallo@apple.com>
    215

  • trunk/Source/JavaScriptCore/ChangeLog

    r280506 r280507  
     12021-07-30  Robin Morisset  <rmorisset@apple.com>
     2
     3        Improve OSR entry into Wasm loops with arguments
     4        https://bugs.webkit.org/show_bug.cgi?id=228595
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        This patch has two parts:
     9        - improve the Wasm OSR code to fully support loop arguments (just some plumbing to make sure that the right values are propagated)
     10        - improve the B3 validator to fix a hole I noticed while writing the first part: we were not detecting code that introduce Upsilons in the wrong blocks.
     11          Naturally, this caused hard to debug issues, as B3 has no well-defined semantics for a Phi that is reached before the corresponding Upsilon(s).
     12
     13        * b3/B3Validate.cpp:
     14        * wasm/WasmAirIRGenerator.cpp:
     15        (JSC::Wasm::AirIRGenerator::emitLoopTierUpCheck):
     16        (JSC::Wasm::AirIRGenerator::addLoop):
     17        * wasm/WasmB3IRGenerator.cpp:
     18        (JSC::Wasm::B3IRGenerator::emitLoopTierUpCheck):
     19        (JSC::Wasm::B3IRGenerator::addLoop):
     20        * wasm/WasmLLIntGenerator.cpp:
     21        (JSC::Wasm::LLIntGenerator::addLoop):
     22
    1232021-07-30  Philip Chimento  <pchimento@igalia.com>
    224

  • trunk/Source/JavaScriptCore/b3/B3Validate.cpp

    r278253 r280507  
    573573            VALIDATE(block->numPredecessors() == predecessors.size(), ("At ", *block));
    574574        }
     575
     576        validatePhisAreDominatedByUpsilons();
    575577    }
    576578
    … …  
    653655        VALIDATE(memory->offset() >= 0, ("At ", *value));
    654656    }
    655    
     657
     658    // A simple backwards analysis to check that we cannot reach a Phi without going through a corresponding Upsilon
     659    // We cannot use the dominator tree, since we are checking that each Phi is dominated by a the set of all of its upsilons, and not by a single node.
     660    void validatePhisAreDominatedByUpsilons()
     661    {
     662        bool changed = true;
     663        BitVector blocksToVisit;
     664        IndexMap<BasicBlock*, HashSet<Value*>> undominatedPhisAtTail(m_procedure.size());
     665        for (BasicBlock* block : m_procedure)
     666            blocksToVisit.set(block->index());
     667        while (changed) {
     668            changed = false;
     669            for (BasicBlock* block : m_procedure.blocksInPostOrder()) {
     670                if (!blocksToVisit.quickClear(block->index()))
     671                    continue;
     672                HashSet<Value*> undominatedPhis = undominatedPhisAtTail[block];
     673                for (unsigned index = block->size()-1; index--;) {
     674                    Value* value = block->at(index);
     675                    switch (value->opcode()) {
     676                    case Upsilon:
     677                        undominatedPhis.remove(value->as<UpsilonValue>()->phi());
     678                        break;
     679                    case Phi:
     680                        VALIDATE(!undominatedPhis.contains(value), ("At ", *value));
     681                        undominatedPhis.add(value);
     682                        break;
     683                    default:
     684                        break;
     685                    }
     686                }
     687                for (BasicBlock* predecessor : block->predecessors()) {
     688                    bool changedSet = false;
     689                    for (Value* phi : undominatedPhis)
     690                        changedSet |= undominatedPhisAtTail[predecessor].add(phi).isNewEntry;
     691                    if (changedSet) {
     692                        blocksToVisit.quickSet(predecessor->index());
     693                        changed = true;
     694                    }
     695                }
     696                if (!block->index())
     697                    VALIDATE(undominatedPhis.isEmpty(), ("Undominated phi at top of entry block: ", **undominatedPhis.begin()));
     698            }
     699        }
     700    }
     701
    656702    NO_RETURN_DUE_TO_CRASH void fail(
    657703        const char* filename, int lineNumber, const char* function, const char* condition,

  • trunk/Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp

    r279341 r280507  
    669669
    670670    void emitEntryTierUpCheck();
    671     void emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack);
     671    void emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack, const Stack& newStack);
    672672
    673673    void emitWriteBarrierForJSWrapper();
    … …  
    28472847}
    28482848
    2849 void AirIRGenerator::emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack)
     2849void AirIRGenerator::emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack, const Stack& newStack)
    28502850{
    28512851    uint32_t outerLoopIndex = this->outerLoopIndex();
    … …  
    28852885    }
    28862886    for (TypedExpression value : enclosingStack)
     2887        patchArgs.append(ConstrainedTmp(value.value(), B3::ValueRep::ColdAny));
     2888    for (TypedExpression value : newStack)
    28872889        patchArgs.append(ConstrainedTmp(value.value(), B3::ValueRep::ColdAny));
    28882890
    … …  
    29372939
    29382940    m_currentBlock = body;
    2939     emitLoopTierUpCheck(loopIndex, enclosingStack);
     2941    emitLoopTierUpCheck(loopIndex, enclosingStack, newStack);
    29402942
    29412943    return { };

  • trunk/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp

    r279341 r280507  
    305305
    306306    void emitEntryTierUpCheck();
    307     void emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack);
     307    void emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack, const Stack& newStack);
    308308
    309309    void emitWriteBarrierForJSWrapper();
    … …  
    20682068}
    20692069
    2070 void B3IRGenerator::emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack)
     2070void B3IRGenerator::emitLoopTierUpCheck(uint32_t loopIndex, const Stack& enclosingStack, const Stack& newStack)
    20712071{
    20722072    uint32_t outerLoopIndex = this->outerLoopIndex();
    … …  
    20942094    }
    20952095    for (TypedExpression value : enclosingStack)
     2096        stackmap.append(value);
     2097    for (TypedExpression value : newStack)
    20962098        stackmap.append(value);
    20972099
    … …  
    21452147
    21462148    block = ControlData(m_proc, origin(), signature, BlockType::Loop, continuation, body);
    2147 
    2148     ExpressionList args;
    2149     {
    2150         unsigned offset = enclosingStack.size() - signature->argumentCount();
    2151         for (unsigned i = 0; i < signature->argumentCount(); ++i) {
    2152             TypedExpression value = enclosingStack.at(offset + i);
    2153             auto* upsilon = m_currentBlock->appendNew<UpsilonValue>(m_proc, origin(), value);
    2154             Value* phi = block.phis[i];
    2155             body->append(phi);
    2156             upsilon->setPhi(phi);
    2157             newStack.constructAndAppend(value.type(), phi);
    2158         }
    2159         enclosingStack.shrink(offset);
     2149    unsigned offset = enclosingStack.size() - signature->argumentCount();
     2150    for (unsigned i = 0; i < signature->argumentCount(); ++i) {
     2151        TypedExpression value = enclosingStack.at(offset + i);
     2152        auto* upsilon = m_currentBlock->appendNew<UpsilonValue>(m_proc, origin(), value);
     2153        Value* phi = block.phis[i];
     2154        body->append(phi);
     2155        upsilon->setPhi(phi);
     2156        newStack.constructAndAppend(value.type(), phi);
    21602157    }
    21612158
    … …  
    22182215            connectControlEntry(data, expressionStack);
    22192216        }
     2217        for (unsigned i = 0; i < signature->argumentCount(); ++i) {
     2218            TypedExpression value = enclosingStack.at(offset + i);
     2219            Value* phi = block.phis[i];
     2220            m_currentBlock->appendNew<UpsilonValue>(m_proc, value->origin(), loadFromScratchBuffer(value->type()), phi);
     2221        }
     2222        enclosingStack.shrink(offset);
    22202223        connectControlEntry(block, enclosingStack);
    22212224
    … …  
    22232226        m_currentBlock->appendNewControlValue(m_proc, Jump, origin(), body);
    22242227        body->addPredecessor(m_currentBlock);
    2225     }
     2228    } else
     2229        enclosingStack.shrink(offset);
    22262230
    22272231    m_currentBlock = body;
    2228     emitLoopTierUpCheck(loopIndex, enclosingStack);
     2232    emitLoopTierUpCheck(loopIndex, enclosingStack, newStack);
    22292233    return { };
    22302234}

  • trunk/Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp

    r279265 r280507  
    923923    for (TypedExpression expression : enclosingStack)
    924924        osrEntryData.append(expression);
     925    for (TypedExpression expression : newStack)
     926        osrEntryData.append(expression);
    925927
    926928    WasmLoopHint::emit(this);
Note: See TracChangeset for help on using the changeset viewer.