[MERGE chakra-core#855] Fix bug with negative 0 handling in global optimizer

rajatd · rajatd · commit 5f0a0a211e6b · 2016-05-03T18:08:10.000-07:00
Merge pull request chakra-core#855 from rajatd:negZero-abortOptOnBailout The forward pass sets a bit, m_wasNegativeZeroPreventedByBailout, on an operand if it was int-type-specialized and was protected by a BailOutOnNegativeZero (which would happen if there are uses of that operand downstream in manners which require distinction between -0 and +0 and the forward pass cannot say for sure that the operand can't be -0). The Deadstore pass then looks at this bit to determine if it can remove some -0 bailouts. However, this bit being set to false for an operand doesn't necessarily mean that that operand cannot be -0 if the operation that produced this operand wasn't type-spec'ed at all. Negative zero tracking in the deadstore pass wasn't taking this into account and was getting rid of more -0 bailouts than it should have, resulting in wrong results. Fix being proposed here is, - for the current instruction, detect the source operand for whose defintion we can remove negative zero bailout. - if there is any bailout instruction between this definition and the current instruction, then don't remove the negative zero bailout. Reasoning behind this is that if we removed -0 bailout for, say, s1 and - a bailout happened between def of s1 and an int specialized use of s1, and - s1 was actually -0 (but we removed the bailout), s1 will be +0 in the interpreter leading to wrong results.
diff --git a/lib/Backend/BackwardPass.cpp b/lib/Backend/BackwardPass.cpp
@@ -249,6 +249,7 @@ BackwardPass::CleanupBackwardPassInfoInFlowGraph()
         block->noImplicitCallNativeArrayUses = nullptr;
         block->noImplicitCallJsArrayHeadSegmentSymUses = nullptr;
         block->noImplicitCallArrayLengthSymUses = nullptr;
+        block->couldRemoveNegZeroBailoutForDef = nullptr;
 
         if (block->loop != nullptr)
         {
@@ -372,6 +373,7 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
     BVSparse<JitArenaAllocator> * fieldHoistCandidates = nullptr;
     BVSparse<JitArenaAllocator> * slotDeadStoreCandidates = nullptr;
     BVSparse<JitArenaAllocator> * byteCodeUpwardExposedUsed = nullptr;
+    BVSparse<JitArenaAllocator> * couldRemoveNegZeroBailoutForDef = nullptr;
 #if DBG
     uint byteCodeLocalsCount = func->GetJnFunction()->GetLocalsCount();
     StackSym ** byteCodeRestoreSyms = nullptr;
@@ -435,6 +437,10 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
             {
                 cloneStrCandidates = JitAnew(this->globOpt->alloc, BVSparse<JitArenaAllocator>, this->globOpt->alloc);
             }
+            else
+            {
+                couldRemoveNegZeroBailoutForDef = JitAnew(this->tempAlloc, BVSparse<JitArenaAllocator>, this->tempAlloc);
+            }
         }
 
         bool firstSucc = true;
@@ -802,6 +808,16 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
                     }
 #endif
                 }
+
+                if (blockSucc->couldRemoveNegZeroBailoutForDef != nullptr)
+                {
+                    couldRemoveNegZeroBailoutForDef->And(blockSucc->couldRemoveNegZeroBailoutForDef);
+                    if (deleteData)
+                    {
+                        JitAdelete(this->tempAlloc, blockSucc->couldRemoveNegZeroBailoutForDef);
+                        blockSucc->couldRemoveNegZeroBailoutForDef = nullptr;
+                    }
+                }
             }
 
             if (blockSucc->noImplicitCallUses != nullptr)
@@ -1002,6 +1018,7 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
             Assert(block->noImplicitCallNativeArrayUses == nullptr);
             Assert(block->noImplicitCallJsArrayHeadSegmentSymUses == nullptr);
             Assert(block->noImplicitCallArrayLengthSymUses == nullptr);
+            Assert(block->couldRemoveNegZeroBailoutForDef == nullptr);
         }
         else
         {
@@ -1053,6 +1070,7 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
     block->noImplicitCallNativeArrayUses = noImplicitCallNativeArrayUses;
     block->noImplicitCallJsArrayHeadSegmentSymUses = noImplicitCallJsArrayHeadSegmentSymUses;
     block->noImplicitCallArrayLengthSymUses = noImplicitCallArrayLengthSymUses;
+    block->couldRemoveNegZeroBailoutForDef = couldRemoveNegZeroBailoutForDef;
 }
 
 ObjTypeGuardBucket
@@ -1220,6 +1238,11 @@ BackwardPass::DeleteBlockData(BasicBlock * block)
         block->byteCodeRestoreSyms = nullptr;
 #endif
     }
+    if (block->couldRemoveNegZeroBailoutForDef != nullptr)
+    {
+        JitAdelete(this->tempAlloc, block->couldRemoveNegZeroBailoutForDef);
+        block->couldRemoveNegZeroBailoutForDef = nullptr;
+    }
 }
 
 void
@@ -4933,6 +4956,26 @@ BackwardPass::TrackBitWiseOrNumberOp(IR::Instr *const instr)
     }
 }
 
+void
+BackwardPass::RemoveNegativeZeroBailout(IR::Instr* instr)
+{
+    Assert(instr->HasBailOutInfo() && (instr->GetBailOutKind() & IR::BailOutOnNegativeZero));
+    IR::BailOutKind bailOutKind = instr->GetBailOutKind();
+    bailOutKind = bailOutKind & ~IR::BailOutOnNegativeZero;
+    if (bailOutKind)
+    {
+        instr->SetBailOutKind(bailOutKind);
+    }
+    else
+    {
+        instr->ClearBailOutInfo();
+        if (preOpBailOutInstrToProcess == instr)
+        {
+            preOpBailOutInstrToProcess = nullptr;
+        }
+    }
+}
+
 void
 BackwardPass::TrackIntUsage(IR::Instr *const instr)
 {
@@ -4967,29 +5010,48 @@ BackwardPass::TrackIntUsage(IR::Instr *const instr)
     if(dstSym)
     {
         // For a dst where the def is in this block, transfer the current info into the instruction
-        if(trackNegativeZero && negativeZeroDoesNotMatterBySymId->TestAndClear(dstSym->m_id))
+        if(trackNegativeZero)
         {
-            instr->ignoreNegativeZero = true;
-            if(tag == Js::DeadStorePhase && instr->HasBailOutInfo())
+            if (negativeZeroDoesNotMatterBySymId->Test(dstSym->m_id))
             {
-                IR::BailOutKind bailOutKind = instr->GetBailOutKind();
-                if(bailOutKind & IR::BailOutOnNegativeZero)
+                instr->ignoreNegativeZero = true;
+            }
+
+            if (tag == Js::DeadStorePhase)
+            {
+                if (negativeZeroDoesNotMatterBySymId->TestAndClear(dstSym->m_id))
                 {
-                    bailOutKind -= IR::BailOutOnNegativeZero;
-                    if(bailOutKind)
+                    if (instr->HasBailOutInfo())
                     {
-                        instr->SetBailOutKind(bailOutKind);
+                        IR::BailOutKind bailOutKind = instr->GetBailOutKind();
+                        if (bailOutKind & IR::BailOutOnNegativeZero)
+                        {
+                            RemoveNegativeZeroBailout(instr);
+                        }
                     }
-                    else
+                }
+                else
+                {
+                    if (instr->HasBailOutInfo())
                     {
-                        instr->ClearBailOutInfo();
-                        if(preOpBailOutInstrToProcess == instr)
+                        if (instr->GetBailOutKind() & IR::BailOutOnNegativeZero)
                         {
-                            preOpBailOutInstrToProcess = nullptr;
+                            if (this->currentBlock->couldRemoveNegZeroBailoutForDef->TestAndClear(dstSym->m_id))
+                            {
+                                RemoveNegativeZeroBailout(instr);
+                            }
                         }
+                        // This instruction could potentially bail out. Hence, we cannot reliably remove negative zero
+                        // bailouts upstream. If we did, and the operation actually produced a -0, and this instruction
+                        // bailed out, we'd use +0 instead of -0 in the interpreter.
+                        this->currentBlock->couldRemoveNegZeroBailoutForDef->ClearAll();
                     }
                 }
             }
+            else
+            {
+                this->negativeZeroDoesNotMatterBySymId->Clear(dstSym->m_id);
+            }
         }
         if(trackIntOverflow)
         {
@@ -5096,38 +5158,41 @@ BackwardPass::TrackIntUsage(IR::Instr *const instr)
                 break;
 
             case Js::OpCode::Add_I4:
+            {
                 Assert(dstSym);
                 Assert(instr->GetSrc1());
                 Assert(instr->GetSrc1()->IsRegOpnd() || instr->GetSrc1()->IsIntConstOpnd());
                 Assert(instr->GetSrc2());
                 Assert(instr->GetSrc2()->IsRegOpnd() || instr->GetSrc2()->IsIntConstOpnd());
 
-                if(instr->ignoreNegativeZero ||
-                    !(instr->GetSrc1()->IsRegOpnd() && instr->GetSrc1()->AsRegOpnd()->m_wasNegativeZeroPreventedByBailout) ||
-                    !(instr->GetSrc2()->IsRegOpnd() && instr->GetSrc2()->AsRegOpnd()->m_wasNegativeZeroPreventedByBailout))
+                if (instr->ignoreNegativeZero ||
+                    (instr->GetSrc1()->IsIntConstOpnd() && instr->GetSrc1()->AsIntConstOpnd()->GetValue() != 0) ||
+                    instr->GetSrc2()->IsIntConstOpnd() && instr->GetSrc2()->AsIntConstOpnd()->GetValue() != 0)
                 {
-                    // -0 does not matter for dst, or this instruction does not generate -0 since one of the srcs is not -0
-                    // (regardless of -0 bailout checks)
                     SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc1());
                     SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc2());
                     break;
                 }
-
+                
                 // -0 + -0 == -0. As long as one src is guaranteed to not be -0, -0 does not matter for the other src. Pick a
                 // src for which to ignore negative zero, based on which sym is last-use. If both syms are last-use, src2 is
                 // picked arbitrarily.
-                if(instr->GetSrc2()->IsRegOpnd() &&
-                    !currentBlock->upwardExposedUses->Test(instr->GetSrc2()->AsRegOpnd()->m_sym->m_id))
-                {
-                    SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc2());
-                    SetNegativeZeroMatters(instr->GetSrc1());
-                }
-                else
+                SetNegativeZeroMatters(instr->GetSrc1());
+                SetNegativeZeroMatters(instr->GetSrc2());
+                if (tag == Js::DeadStorePhase)
                 {
-                    SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc1());
-                    SetNegativeZeroMatters(instr->GetSrc2());
+                    if (instr->GetSrc2()->IsRegOpnd() &&
+                        !currentBlock->upwardExposedUses->Test(instr->GetSrc2()->AsRegOpnd()->m_sym->m_id))
+                    {
+                        SetCouldRemoveNegZeroBailoutForDefIfLastUse(instr->GetSrc2());
+                    }
+                    else
+                    {
+                        SetCouldRemoveNegZeroBailoutForDefIfLastUse(instr->GetSrc1());
+                    }
                 }
                 break;
+            }
 
             case Js::OpCode::Add_A:
                 Assert(dstSym);
@@ -5149,26 +5214,26 @@ BackwardPass::TrackIntUsage(IR::Instr *const instr)
                 break;
 
             case Js::OpCode::Sub_I4:
+            {
                 Assert(dstSym);
                 Assert(instr->GetSrc1());
                 Assert(instr->GetSrc1()->IsRegOpnd() || instr->GetSrc1()->IsIntConstOpnd());
                 Assert(instr->GetSrc2());
                 Assert(instr->GetSrc2()->IsRegOpnd() || instr->GetSrc2()->IsIntConstOpnd());
 
-                if(instr->ignoreNegativeZero ||
-                    !(instr->GetSrc1()->IsRegOpnd() && instr->GetSrc1()->AsRegOpnd()->m_wasNegativeZeroPreventedByBailout) ||
+                if (instr->ignoreNegativeZero ||
+                    (instr->GetSrc1()->IsIntConstOpnd() && instr->GetSrc1()->AsIntConstOpnd()->GetValue() != 0) ||
                     instr->GetSrc2()->IsIntConstOpnd() && instr->GetSrc2()->AsIntConstOpnd()->GetValue() != 0)
                 {
-                    // At least one of the following is true:
-                    //     - -0 does not matter for dst
-                    //     - Src1 is not -0 (regardless of -0 bailout checks), and so this instruction cannot generate -0
-                    //     - Src2 is a nonzero int constant, and so this instruction cannot generate -0
                     SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc1());
                     SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc2());
-                    break;
                 }
-                goto NegativeZero_Sub_Default;
-
+                else
+                {
+                    goto NegativeZero_Sub_Default;
+                }
+                break;
+            }
             case Js::OpCode::Sub_A:
                 Assert(dstSym);
                 Assert(instr->GetSrc1());
@@ -5197,7 +5262,11 @@ BackwardPass::TrackIntUsage(IR::Instr *const instr)
             NegativeZero_Sub_Default:
                 // -0 - 0 == -0. As long as src1 is guaranteed to not be -0, -0 does not matter for src2.
                 SetNegativeZeroMatters(instr->GetSrc1());
-                SetNegativeZeroDoesNotMatterIfLastUse(instr->GetSrc2());
+                SetNegativeZeroMatters(instr->GetSrc2());
+                if (this->tag == Js::DeadStorePhase)
+                {
+                    SetCouldRemoveNegZeroBailoutForDefIfLastUse(instr->GetSrc2());
+                }
                 break;
 
             case Js::OpCode::BrEq_I4:
@@ -5628,6 +5697,16 @@ BackwardPass::SetNegativeZeroMatters(IR::Opnd *const opnd)
     }
 }
 
+void
+BackwardPass::SetCouldRemoveNegZeroBailoutForDefIfLastUse(IR::Opnd *const opnd)
+{
+    StackSym * stackSym = IR::RegOpnd::TryGetStackSym(opnd);
+    if (stackSym && !this->currentBlock->upwardExposedUses->Test(stackSym->m_id))
+    {
+        this->currentBlock->couldRemoveNegZeroBailoutForDef->Set(stackSym->m_id);
+    }
+}
+
 void
 BackwardPass::SetIntOverflowDoesNotMatterIfLastUse(IR::Opnd *const opnd)
 {
diff --git a/lib/Backend/BackwardPass.h b/lib/Backend/BackwardPass.h
@@ -77,8 +77,10 @@ class BackwardPass
     void SetSymIsNotUsedOnlyInNumber(IR::Opnd *const opnd);
     void SetSymIsUsedOnlyInNumberIfLastUse(IR::Opnd *const opnd);
     void TrackIntUsage(IR::Instr *const instr);
+    void RemoveNegativeZeroBailout(IR::Instr* instr);
     void SetNegativeZeroDoesNotMatterIfLastUse(IR::Opnd *const opnd);
     void SetNegativeZeroMatters(IR::Opnd *const opnd);
+    void SetCouldRemoveNegZeroBailoutForDefIfLastUse(IR::Opnd *const opnd);
     void SetIntOverflowDoesNotMatterIfLastUse(IR::Opnd *const opnd);
     void SetIntOverflowMatters(IR::Opnd *const opnd);
     bool SetIntOverflowDoesNotMatterInRangeIfLastUse(IR::Opnd *const opnd, const int addSubUses);
diff --git a/lib/Backend/FlowGraph.h b/lib/Backend/FlowGraph.h
@@ -375,6 +375,7 @@ class BasicBlock
     BVSparse<JitArenaAllocator> *           noImplicitCallJsArrayHeadSegmentSymUses;
     BVSparse<JitArenaAllocator> *           noImplicitCallArrayLengthSymUses;
     BVSparse<JitArenaAllocator> *           cloneStrCandidates;
+    BVSparse<JitArenaAllocator> *           couldRemoveNegZeroBailoutForDef; // Deadstore pass only
     Loop * backwardPassCurrentLoop;
 
     // Global optimizer data
@@ -417,6 +418,7 @@ class BasicBlock
         noImplicitCallJsArrayHeadSegmentSymUses(nullptr),
         noImplicitCallArrayLengthSymUses(nullptr),
         cloneStrCandidates(nullptr),
+        couldRemoveNegZeroBailoutForDef(nullptr),
         byteCodeUpwardExposedUsed(nullptr),
         isAirLockCompensationBlock(false),
 #if DBG
diff --git a/test/Optimizer/negativeZero_bugs.baseline b/test/Optimizer/negativeZero_bugs.baseline
@@ -0,0 +1,4 @@
+2
+1
+2
+1
diff --git a/test/Optimizer/negativeZero_bugs.js b/test/Optimizer/negativeZero_bugs.js
@@ -0,0 +1,40 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+function test0(addOrSub) {
+  function makeArrayLength() {
+  }
+  var obj0 = {};
+  var obj1 = {};
+  var protoObj1 = {};
+  var arrObj0 = {};
+
+  var func4 = function () {
+    return arrObj0 * (f > obj1.prop1 ? h++ : Function());
+  };
+  
+  var f = 1;
+  var h = 0;
+  obj1.prop1 = -1;
+  switch(addOrSub)
+  {
+    case 1:
+      f /= ((1 - 1) * -1) - -(func4.call(arrObj0) << (typeof arrObj0.length == null));
+      break;
+
+    case 2:
+      f /= ((1 - 1) * -1) + -(func4.call(arrObj0) << (typeof arrObj0.length == null));
+      break;
+  }
+  
+  func4();
+  
+  print(h);
+}
+test0(1);
+test0(2);
+
+test0(1);
+test0(2);
diff --git a/test/Optimizer/rlexe.xml b/test/Optimizer/rlexe.xml
@@ -1366,4 +1366,11 @@
       <baseline>bug7100343.baseline</baseline>
     </default>
   </test>
+  <test>
+    <default>
+      <files>negativeZero_bugs.js</files>
+      <compile-flags>-mic:2 -off:simplejit</compile-flags>
+      <baseline>negativeZero_bugs.baseline</baseline>
+    </default>
+  </test>
 </regress-exe>

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +2
 +1
 +2
 +1