FTL B3 should know that used registers are not the same thing as used registers....
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Dec 2015 22:46:51 +0000 (22:46 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Dec 2015 22:46:51 +0000 (22:46 +0000)
latter to unavailable registers to avoid future confusion.
https://bugs.webkit.org/show_bug.cgi?id=152572

Reviewed by Saam Barati.

Prior to this change, we used the term "used registers" in two different senses:

- The set of registers that are live at some point in the current compilation unit. A
  register is live at some point if it is read after that point on some path through that
  point.

- The set of registers that are not available for scratch register use at some point. A
  register may not be available if it is live or if it is a callee-save register but it is
  not being saved by the current compilation.

In the old FTL LLVM code, we had some translations from the first sense into the second
sense. We forgot to do those in FTL B3, and so we get crashes, for example in V8/splay. That
benchmark highlighted this issue because it fired some lazy slow paths, and then used an
unsaved callee-save for scratch.

Curiously, we could merge these two definitions by observing that, in some sense, an unsaved
callee save is live at every point in a compilation in the sense that it may contain a value
that will be read when the compilation returns. That's pretty cool, but it feels strange to
me. This isn't how we would normally define liveness of registers. It's not how the
Air::TmpLiveness analysis would do it for any of its other clients.

So, this changes B3 to have two different concepts:

- Used registers. These are the registers that are live.

- Unavailable registers. These are the registers that are not available for scratch. It's
  always a superset of used registers.

This also changes FTLLower to use unavailableRegisters() pretty much everywhere that it
previously used usedRegisters().

This makes it possible to run V8/splay.

* b3/B3StackmapGenerationParams.cpp:
(JSC::B3::StackmapGenerationParams::usedRegisters):
(JSC::B3::StackmapGenerationParams::unavailableRegisters):
(JSC::B3::StackmapGenerationParams::proc):
* b3/B3StackmapGenerationParams.h:
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compilePutById):
(JSC::FTL::DFG::LowerDFGToLLVM::getById):
(JSC::FTL::DFG::LowerDFGToLLVM::lazySlowPath):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@194431 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp
Source/JavaScriptCore/b3/B3StackmapGenerationParams.h
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

index 61f5bfd..7046e7a 100644 (file)
@@ -1,3 +1,54 @@
+2015-12-27  Filip Pizlo  <fpizlo@apple.com>
+
+        FTL B3 should know that used registers are not the same thing as used registers. Rename the
+        latter to unavailable registers to avoid future confusion.
+        https://bugs.webkit.org/show_bug.cgi?id=152572
+
+        Reviewed by Saam Barati.
+
+        Prior to this change, we used the term "used registers" in two different senses:
+
+        - The set of registers that are live at some point in the current compilation unit. A
+          register is live at some point if it is read after that point on some path through that
+          point.
+
+        - The set of registers that are not available for scratch register use at some point. A
+          register may not be available if it is live or if it is a callee-save register but it is
+          not being saved by the current compilation.
+
+        In the old FTL LLVM code, we had some translations from the first sense into the second
+        sense. We forgot to do those in FTL B3, and so we get crashes, for example in V8/splay. That
+        benchmark highlighted this issue because it fired some lazy slow paths, and then used an
+        unsaved callee-save for scratch.
+        Curiously, we could merge these two definitions by observing that, in some sense, an unsaved
+        callee save is live at every point in a compilation in the sense that it may contain a value
+        that will be read when the compilation returns. That's pretty cool, but it feels strange to
+        me. This isn't how we would normally define liveness of registers. It's not how the
+        Air::TmpLiveness analysis would do it for any of its other clients.
+
+        So, this changes B3 to have two different concepts:
+
+        - Used registers. These are the registers that are live.
+
+        - Unavailable registers. These are the registers that are not available for scratch. It's
+          always a superset of used registers.
+
+        This also changes FTLLower to use unavailableRegisters() pretty much everywhere that it
+        previously used usedRegisters().
+
+        This makes it possible to run V8/splay.
+
+        * b3/B3StackmapGenerationParams.cpp:
+        (JSC::B3::StackmapGenerationParams::usedRegisters):
+        (JSC::B3::StackmapGenerationParams::unavailableRegisters):
+        (JSC::B3::StackmapGenerationParams::proc):
+        * b3/B3StackmapGenerationParams.h:
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::compilePutById):
+        (JSC::FTL::DFG::LowerDFGToLLVM::getById):
+        (JSC::FTL::DFG::LowerDFGToLLVM::lazySlowPath):
+
 2015-12-25  Andy Estes  <aestes@apple.com>
 
         Stop moving local objects in return statements
index 6114cb0..ebd7524 100644 (file)
@@ -41,6 +41,18 @@ const RegisterSet& StackmapGenerationParams::usedRegisters() const
     return m_value->m_usedRegisters;
 }
 
+RegisterSet StackmapGenerationParams::unavailableRegisters() const
+{
+    RegisterSet result = usedRegisters();
+    
+    RegisterSet unsavedCalleeSaves = RegisterSet::vmCalleeSaveRegisters();
+    for (const RegisterAtOffset& regAtOffset : m_context.code->calleeSaveRegisters())
+        unsavedCalleeSaves.clear(regAtOffset.reg());
+
+    result.merge(unsavedCalleeSaves);
+    return result;
+}
+
 Procedure& StackmapGenerationParams::proc() const
 {
     return m_context.code->proc();
index e6bc3f5..09055c2 100644 (file)
@@ -59,6 +59,13 @@ public:
     // This tells you the registers that were used.
     const RegisterSet& usedRegisters() const;
 
+    // This is a useful helper if you want to do register allocation inside of a patchpoint. You
+    // can only use callee-save registers if they were saved in the prologue. This gives you the
+    // used register set that's useful for such settings by returning:
+    //
+    //     usedRegisters() | (RegisterSet::calleeSaveRegisters() - proc.calleeSaveRegisters())
+    RegisterSet unavailableRegisters() const;
+
     // This is provided for convenience; it means that you don't have to capture it if you don't want to.
     Procedure& proc() const;
     
index b182fbb..febc225 100644 (file)
@@ -2586,8 +2586,8 @@ private:
                 auto generator = Box<JITPutByIdGenerator>::create(
                     jit.codeBlock(), node->origin.semantic,
                     state->jitCode->common.addUniqueCallSiteIndex(node->origin.semantic),
-                    params.usedRegisters(), JSValueRegs(params[0].gpr()), JSValueRegs(params[1].gpr()),
-                    GPRInfo::patchpointScratchRegister, ecmaMode,
+                    params.unavailableRegisters(), JSValueRegs(params[0].gpr()),
+                    JSValueRegs(params[1].gpr()), GPRInfo::patchpointScratchRegister, ecmaMode,
                     node->op() == PutByIdDirect ? Direct : NotDirect);
 
                 generator->generateFastPath(jit);
@@ -2603,8 +2603,8 @@ private:
                         generator->slowPathJump().link(&jit);
                         CCallHelpers::Label slowPathBegin = jit.label();
                         CCallHelpers::Call slowPathCall = callOperation(
-                            *state, params.usedRegisters(), jit, node->origin.semantic, &exceptions,
-                            generator->slowPathFunction(), InvalidGPRReg,
+                            *state, params.unavailableRegisters(), jit, node->origin.semantic,
+                            &exceptions, generator->slowPathFunction(), InvalidGPRReg,
                             CCallHelpers::TrustedImmPtr(generator->stubInfo()), params[1].gpr(),
                             params[0].gpr(), CCallHelpers::TrustedImmPtr(uid)).call();
                         jit.jump().linkTo(done, &jit);
@@ -7091,7 +7091,8 @@ private:
                 auto generator = Box<JITGetByIdGenerator>::create(
                     jit.codeBlock(), node->origin.semantic,
                     state->jitCode->common.addUniqueCallSiteIndex(node->origin.semantic),
-                    params.usedRegisters(), JSValueRegs(params[1].gpr()), JSValueRegs(params[0].gpr()));
+                    params.unavailableRegisters(), JSValueRegs(params[1].gpr()),
+                    JSValueRegs(params[0].gpr()));
 
                 generator->generateFastPath(jit);
                 CCallHelpers::Label done = jit.label();
@@ -7106,8 +7107,8 @@ private:
                         generator->slowPathJump().link(&jit);
                         CCallHelpers::Label slowPathBegin = jit.label();
                         CCallHelpers::Call slowPathCall = callOperation(
-                            *state, params.usedRegisters(), jit, node->origin.semantic, &exceptions,
-                            operationGetByIdOptimize, params[0].gpr(),
+                            *state, params.unavailableRegisters(), jit, node->origin.semantic,
+                            &exceptions, operationGetByIdOptimize, params[0].gpr(),
                             CCallHelpers::TrustedImmPtr(generator->stubInfo()), params[1].gpr(),
                             CCallHelpers::TrustedImmPtr(uid)).call();
                         jit.jump().linkTo(done, &jit);
@@ -8516,7 +8517,7 @@ private:
                 CCallHelpers::PatchableJump patchableJump = jit.patchableJump();
                 CCallHelpers::Label done = jit.label();
 
-                RegisterSet usedRegisters = params.usedRegisters();
+                RegisterSet usedRegisters = params.unavailableRegisters();
 
                 // FIXME: As part of handling exceptions, we need to create a concrete OSRExit here.
                 // Doing so should automagically register late paths that emit exit thunks.
@@ -8544,7 +8545,7 @@ private:
                                     generatorJump, CodeLocationLabel(
                                         vm->getCTIStub(
                                             lazySlowPathGenerationThunkGenerator).code()));
-                                    
+                                
                                 CodeLocationJump linkedPatchableJump = CodeLocationJump(
                                     linkBuffer.locationOf(patchableJump));
                                 CodeLocationLabel linkedDone = linkBuffer.locationOf(done);