Title: [212018] branches/safari-603-branch/Source/_javascript_Core

Diff

Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (212017 => 212018)


--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog	2017-02-10 01:49:38 UTC (rev 212017)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog	2017-02-10 01:49:41 UTC (rev 212018)
@@ -1,5 +1,39 @@
 2017-02-09  Matthew Hanson  <matthew_han...@apple.com>
 
+        Merge r211896. rdar://problem/29754721
+
+    2017-02-08  Saam Barati  <sbar...@apple.com>
+
+            Air IRC might spill a terminal that produces a value after the terminal
+            https://bugs.webkit.org/show_bug.cgi?id=167919
+            <rdar://problem/29754721>
+
+            Reviewed by Filip Pizlo.
+
+            IRC may spill a value-producing terminal (a patchpoint can be a value-producing terminal).
+            It used to do this by placing the spill *after* the terminal. This produces an invalid
+            graph because no instructions are allowed after the terminal.
+
+            I fixed this bug by having a cleanup pass over the IR after IRC is done.
+            The pass detects this problem, and fixes it by moving the spill into the
+            successors. However, it is careful to detect when the edge to the
+            successor is a critical edge. If the value-producing patchpoint is
+            the only predecessor of the successor, it just moves the spill
+            code to the beginning of the successor. Otherwise, it's a critical
+            edge and it breaks it by adding a block that does the spilling then
+            jumps to the successor.
+
+            * b3/air/AirInsertionSet.cpp:
+            * b3/air/AirInsertionSet.h:
+            (JSC::B3::Air::InsertionSet::insertInsts):
+            * b3/air/AirIteratedRegisterCoalescing.cpp:
+            * b3/testb3.cpp:
+            (JSC::B3::testTerminalPatchpointThatNeedsToBeSpilled):
+            (JSC::B3::testTerminalPatchpointThatNeedsToBeSpilled2):
+            (JSC::B3::run):
+
+2017-02-09  Matthew Hanson  <matthew_han...@apple.com>
+
         Merge r211642. rdar://problem/29542720
 
     2017-02-03  Saam Barati  <sbar...@apple.com>

Modified: branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.cpp (212017 => 212018)


--- branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.cpp	2017-02-10 01:49:38 UTC (rev 212017)
+++ branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.cpp	2017-02-10 01:49:41 UTC (rev 212018)
@@ -33,12 +33,6 @@
 
 namespace JSC { namespace B3 { namespace Air {
 
-void InsertionSet::insertInsts(size_t index, const Vector<Inst>& insts)
-{
-    for (const Inst& inst : insts)
-        insertInst(index, inst);
-}
-
 void InsertionSet::insertInsts(size_t index, Vector<Inst>&& insts)
 {
     for (Inst& inst : insts)

Modified: branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.h (212017 => 212018)


--- branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.h	2017-02-10 01:49:38 UTC (rev 212017)
+++ branches/safari-603-branch/Source/_javascript_Core/b3/air/AirInsertionSet.h	2017-02-10 01:49:41 UTC (rev 212018)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -59,7 +59,12 @@
         appendInsertion(Insertion(index, std::forward<Inst>(inst)));
     }
 
-    void insertInsts(size_t index, const Vector<Inst>&);
+    template <typename InstVector>
+    void insertInsts(size_t index, const InstVector& insts)
+    {
+        for (const Inst& inst : insts)
+            insertInst(index, inst);
+    }
     void insertInsts(size_t index, Vector<Inst>&&);
     
     template<typename... Arguments>

Modified: branches/safari-603-branch/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp (212017 => 212018)


--- branches/safari-603-branch/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp	2017-02-10 01:49:38 UTC (rev 212017)
+++ branches/safari-603-branch/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp	2017-02-10 01:49:41 UTC (rev 212018)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -1274,6 +1274,8 @@
         iteratedRegisterCoalescingOnType<Arg::GP>();
         iteratedRegisterCoalescingOnType<Arg::FP>();
 
+        fixSpillsAfterTerminals();
+
         if (reportStats)
             dataLog("Num iterations = ", m_numIterations, "\n");
     }
@@ -1573,6 +1575,66 @@
         }
     }
 
+    void fixSpillsAfterTerminals()
+    {
+        // Because there may be terminals that produce values, IRC may
+        // want to spill those terminals. It'll happen to spill it after
+        // the terminal. If we left the graph in this state, it'd be invalid
+        // because a terminal must be the last instruction in a block.
+        // We fix that here.
+
+        InsertionSet insertionSet(m_code);
+
+        bool addedBlocks = false;
+
+        for (BasicBlock* block : m_code) {
+            unsigned terminalIndex = block->size();
+            bool foundTerminal = false;
+            while (terminalIndex--) {
+                if (block->at(terminalIndex).isTerminal()) {
+                    foundTerminal = true;
+                    break;
+                }
+            }
+            ASSERT_UNUSED(foundTerminal, foundTerminal);
+
+            if (terminalIndex == block->size() - 1)
+                continue;
+
+            // There must be instructions after the terminal because it's not the last instruction.
+            ASSERT(terminalIndex < block->size() - 1);
+            Vector<Inst, 1> instsToMove;
+            for (unsigned i = terminalIndex + 1; i < block->size(); i++)
+                instsToMove.append(block->at(i));
+            RELEASE_ASSERT(instsToMove.size());
+
+            for (FrequentedBlock& frequentedSuccessor : block->successors()) {
+                BasicBlock* successor = frequentedSuccessor.block();
+                // If successor's only predecessor is block, we can plant the spill inside
+                // the successor. Otherwise, we must split the critical edge and create
+                // a new block for the spill.
+                if (successor->numPredecessors() == 1) {
+                    insertionSet.insertInsts(0, instsToMove);
+                    insertionSet.execute(successor);
+                } else {
+                    addedBlocks = true;
+                    // FIXME: We probably want better block ordering here.
+                    BasicBlock* newBlock = m_code.addBlock();
+                    for (const Inst& inst : instsToMove)
+                        newBlock->appendInst(inst);
+                    newBlock->appendInst(Inst(Jump, instsToMove.last().origin));
+                    newBlock->successors().append(successor);
+                    frequentedSuccessor.block() = newBlock;
+                }
+            }
+
+            block->resize(terminalIndex + 1);
+        }
+
+        if (addedBlocks)
+            m_code.resetReachability();
+    }
+
     Code& m_code;
     TmpWidth m_tmpWidth;
     UseCounts<Tmp> m_useCounts;

Modified: branches/safari-603-branch/Source/_javascript_Core/b3/testb3.cpp (212017 => 212018)


--- branches/safari-603-branch/Source/_javascript_Core/b3/testb3.cpp	2017-02-10 01:49:38 UTC (rev 212017)
+++ branches/safari-603-branch/Source/_javascript_Core/b3/testb3.cpp	2017-02-10 01:49:41 UTC (rev 212018)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -13199,6 +13199,166 @@
     CHECK(terminal.args[2].kind() == Air::Arg::BitImm || terminal.args[2].kind() == Air::Arg::BitImm64);
 }
 
+void testTerminalPatchpointThatNeedsToBeSpilled()
+{
+    // This is a unit test for how FTL's heap allocation fast paths behave.
+    Procedure proc;
+    
+    BasicBlock* root = proc.addBlock();
+    BasicBlock* success = proc.addBlock();
+    BasicBlock* slowPath = proc.addBlock();
+    
+    PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+    patchpoint->effects.terminal = true;
+    patchpoint->clobber(RegisterSet::macroScratchRegisters());
+    
+    root->appendSuccessor(success);
+    root->appendSuccessor(FrequentedBlock(slowPath, FrequencyClass::Rare));
+    
+    patchpoint->setGenerator(
+        [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+            AllowMacroScratchRegisterUsage allowScratch(jit);
+            jit.move(CCallHelpers::TrustedImm32(42), params[0].gpr());
+            
+            CCallHelpers::Jump jumpToSuccess;
+            if (!params.fallsThroughToSuccessor(0))
+                jumpToSuccess = jit.jump();
+            
+            Vector<Box<CCallHelpers::Label>> labels = params.successorLabels();
+            
+            params.addLatePath(
+                [=] (CCallHelpers& jit) {
+                    if (jumpToSuccess.isSet())
+                        jumpToSuccess.linkTo(*labels[0], &jit);
+                });
+        });
+    
+    Vector<Value*> args;
+    {
+        RegisterSet fillAllGPRsSet = RegisterSet::allGPRs();
+        fillAllGPRsSet.exclude(RegisterSet::stackRegisters());
+        fillAllGPRsSet.exclude(RegisterSet::reservedHardwareRegisters());
+
+        for (unsigned i = 0; i < fillAllGPRsSet.numberOfSetRegisters(); i++)
+            args.append(success->appendNew<Const32Value>(proc, Origin(), i));
+    }
+
+    {
+        // Now force all values into every available register.
+        PatchpointValue* p = success->appendNew<PatchpointValue>(proc, Void, Origin());
+        for (Value* v : args)
+            p->append(v, ValueRep::SomeRegister);
+        p->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
+    {
+        // Now require the original patchpoint to be materialized into a register.
+        PatchpointValue* p = success->appendNew<PatchpointValue>(proc, Void, Origin());
+        p->append(patchpoint, ValueRep::SomeRegister);
+        p->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
+    success->appendNew<Value>(proc, Return, Origin(), success->appendNew<Const32Value>(proc, Origin(), 10));
+    
+    slowPath->appendNew<Value>(proc, Return, Origin(), slowPath->appendNew<Const32Value>(proc, Origin(), 20));
+    
+    auto code = compile(proc);
+    CHECK_EQ(invoke<int>(*code), 10);
+}
+
+void testTerminalPatchpointThatNeedsToBeSpilled2()
+{
+    // This is a unit test for how FTL's heap allocation fast paths behave.
+    Procedure proc;
+    
+    BasicBlock* root = proc.addBlock();
+    BasicBlock* _one_ = proc.addBlock();
+    BasicBlock* success = proc.addBlock();
+    BasicBlock* slowPath = proc.addBlock();
+
+    Value* arg = root->appendNew<Value>(
+        proc, Trunc, Origin(),
+        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
+
+    root->appendNew<Value>(
+        proc, Branch, Origin(), arg);
+    root->appendSuccessor(one);
+    root->appendSuccessor(FrequentedBlock(slowPath, FrequencyClass::Rare));
+    
+    PatchpointValue* patchpoint = one->appendNew<PatchpointValue>(proc, Int32, Origin());
+    patchpoint->effects.terminal = true;
+    patchpoint->clobber(RegisterSet::macroScratchRegisters());
+    patchpoint->append(arg, ValueRep::SomeRegister);
+    
+    one->appendSuccessor(success);
+    one->appendSuccessor(FrequentedBlock(slowPath, FrequencyClass::Rare));
+    
+    patchpoint->setGenerator(
+        [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+            AllowMacroScratchRegisterUsage allowScratch(jit);
+            jit.move(CCallHelpers::TrustedImm32(666), params[0].gpr());
+            auto goToFastPath = jit.branch32(CCallHelpers::Equal, params[1].gpr(), CCallHelpers::TrustedImm32(42));
+            auto jumpToSlow = jit.jump();
+            
+            // Make sure the asserts here pass.
+            params.fallsThroughToSuccessor(0);
+            params.fallsThroughToSuccessor(1);
+
+            Vector<Box<CCallHelpers::Label>> labels = params.successorLabels();
+            
+            params.addLatePath(
+                [=] (CCallHelpers& jit) {
+                    goToFastPath.linkTo(*labels[0], &jit);
+                    jumpToSlow.linkTo(*labels[1], &jit);
+                });
+        });
+    
+    Vector<Value*> args;
+    {
+        RegisterSet fillAllGPRsSet = RegisterSet::allGPRs();
+        fillAllGPRsSet.exclude(RegisterSet::stackRegisters());
+        fillAllGPRsSet.exclude(RegisterSet::reservedHardwareRegisters());
+
+        for (unsigned i = 0; i < fillAllGPRsSet.numberOfSetRegisters(); i++)
+            args.append(success->appendNew<Const32Value>(proc, Origin(), i));
+    }
+
+    {
+        // Now force all values into every available register.
+        PatchpointValue* p = success->appendNew<PatchpointValue>(proc, Void, Origin());
+        for (Value* v : args)
+            p->append(v, ValueRep::SomeRegister);
+        p->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
+    {
+        // Now require the original patchpoint to be materialized into a register.
+        PatchpointValue* p = success->appendNew<PatchpointValue>(proc, Void, Origin());
+        p->append(patchpoint, ValueRep::SomeRegister);
+        p->setGenerator([&] (CCallHelpers&, const StackmapGenerationParams&) { });
+    }
+
+    success->appendNew<Value>(proc, Return, Origin(), patchpoint);
+    
+    slowPath->appendNew<Value>(proc, Return, Origin(), arg);
+    
+    auto original1 = Options::maxB3TailDupBlockSize();
+    auto original2 = Options::maxB3TailDupBlockSuccessors();
+
+    // Tail duplication will break the critical edge we're trying to test because it
+    // will clone the slowPath block for both edges to it!
+    Options::maxB3TailDupBlockSize() = 0;
+    Options::maxB3TailDupBlockSuccessors() = 0;
+
+    auto code = compile(proc);
+    CHECK_EQ(invoke<int>(*code, 1), 1);
+    CHECK_EQ(invoke<int>(*code, 0), 0);
+    CHECK_EQ(invoke<int>(*code, 42), 666);
+
+    Options::maxB3TailDupBlockSize() = original1;
+    Options::maxB3TailDupBlockSuccessors() = original2;
+}
+
 void testPatchpointTerminalReturnValue(bool successIsRare)
 {
     // This is a unit test for how FTL's heap allocation fast paths behave.
@@ -14083,6 +14243,10 @@
         return !filter || !!strcasestr(testName, filter);
     };
 
+    // We run this test first because it fiddles with some
+    // JSC options.
+    testTerminalPatchpointThatNeedsToBeSpilled2();
+
     RUN(test42());
     RUN(testLoad42());
     RUN(testLoadOffsetImm9Max());
@@ -15458,6 +15622,7 @@
     RUN(testSomeEarlyRegister());
     RUN(testPatchpointTerminalReturnValue(true));
     RUN(testPatchpointTerminalReturnValue(false));
+    RUN(testTerminalPatchpointThatNeedsToBeSpilled());
 
     RUN(testMemoryFence());
     RUN(testStoreFence());
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to