Temporary GPR should not be lazily allocated in DFG JIT on X86
authoryuqiang.xian@intel.com <yuqiang.xian@intel.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Dec 2011 07:31:00 +0000 (07:31 +0000)
committeryuqiang.xian@intel.com <yuqiang.xian@intel.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Dec 2011 07:31:00 +0000 (07:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=74908

Reviewed by Filip Pizlo.

On X86, we used to allocate a temporary GPR lazily when it's really
used rather than defined. This may cause potential issues of
allocating registers inside control flow and result in problems in
subsequent code generation, for example the DFG JIT may think an
operand already being spilled (to satisfy the allocation request) and
generate code to read the data from memory, but the allocation and
spilling are in a branch which is not taken at runtime, so the
generated code is incorrect.

Although current DFG JIT code doesn't have this problematic pattern,
it's better to cut-off the root to avoid any potential issues in the
future.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::GPRTemporary::GPRTemporary):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::GPRTemporary::gpr):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp

index 55ec58d..e007dc3 100644 (file)
@@ -1,5 +1,32 @@
 2011-12-19  Yuqiang Xian  <yuqiang.xian@intel.com>
 
+        Temporary GPR should not be lazily allocated in DFG JIT on X86
+        https://bugs.webkit.org/show_bug.cgi?id=74908
+
+        Reviewed by Filip Pizlo.
+
+        On X86, we used to allocate a temporary GPR lazily when it's really
+        used rather than defined. This may cause potential issues of
+        allocating registers inside control flow and result in problems in
+        subsequent code generation, for example the DFG JIT may think an
+        operand already being spilled (to satisfy the allocation request) and
+        generate code to read the data from memory, but the allocation and
+        spilling are in a branch which is not taken at runtime, so the
+        generated code is incorrect.
+
+        Although current DFG JIT code doesn't have this problematic pattern,
+        it's better to cut-off the root to avoid any potential issues in the
+        future.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::GPRTemporary::GPRTemporary):
+        * dfg/DFGSpeculativeJIT.h:
+        (JSC::DFG::GPRTemporary::gpr):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+
+2011-12-19  Yuqiang Xian  <yuqiang.xian@intel.com>
+
         Remove unused code for non-speculative Arith operations from DFG JIT
         https://bugs.webkit.org/show_bug.cgi?id=74905
 
index bd057e0..9759380 100644 (file)
@@ -564,11 +564,7 @@ GPRTemporary::GPRTemporary(SpeculativeJIT* jit)
     : m_jit(jit)
     , m_gpr(InvalidGPRReg)
 {
-#if CPU(X86)
-    // we currenty lazily allocate the reg, as the number of regs on X86 is limited.
-#else
     m_gpr = m_jit->allocate();
-#endif
 }
 
 GPRTemporary::GPRTemporary(SpeculativeJIT* jit, GPRReg specific)
index 67c57bb..0841129 100644 (file)
@@ -2434,9 +2434,6 @@ public:
 
     GPRReg gpr()
     {
-        // In some cases we have lazy allocation.
-        if (m_jit && m_gpr == InvalidGPRReg)
-            m_gpr = m_jit->allocate();
         return m_gpr;
     }
 
index 9f2a146..957dca7 100644 (file)
@@ -2294,9 +2294,6 @@ void SpeculativeJIT::compile(Node& node)
 
         SpeculateStrictInt32Operand property(this, node.child2());
         StorageOperand storage(this, node.child3());
-        GPRTemporary resultTag(this);
-        GPRTemporary resultPayload(this);
-        
         GPRReg propertyReg = property.gpr();
         GPRReg storageReg = storage.gpr();
         
@@ -2313,6 +2310,9 @@ void SpeculativeJIT::compile(Node& node)
             speculationCheck(Uncountable, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, MacroAssembler::Address(baseReg, JSArray::vectorLengthOffset())));
         }
 
+        GPRTemporary resultTag(this);
+        GPRTemporary resultPayload(this);
+
         // FIXME: In cases where there are subsequent by_val accesses to the same base it might help to cache
         // the storage pointer - especially if there happens to be another register free right now. If we do so,
         // then we'll need to allocate a new temporary for result.