[1.8>master] [MERGE #4447 @rajatd] Fixing incremental capturing of values (for bailout) in GlobOpt

Merge pull request #4447 from rajatd:capturedValues

Due to multiple issues with incremental capturing of values in globopt, we were never capturing values incrementally across blocks. This PR fixes those issues and makes capturing values across blocks incremental (for real :) ).

`GlobOptBlockData.cpp:`
Fixes to propagate captured values to the successor in case of a single predecessor and to merge correctly the values captured on all the incoming edges, i.e., predecessors

`GlobOptBailout.cpp:`
Refactoring the code for incremental value capture and fixing the case when there were captured syms which were unprocessed after going over all the syms which were changed since the last time we captured values for bailout.

Gives a nice win in Octane on arm64 (fyi @agarwal-sandeep @sigatrev @Penguinwizzard @aaronsgiles):

```
Octane            Left score       Right score      ∆ Score  ∆ Score %  Comment
----------------  ---------------  ---------------  -------  ---------  ---------------
Box2d              6419.45 ±0.23%   7029.38 ±1.53%   609.92      9.50%  Improved
Code-load          8963.50 ±0.91%   8906.50 ±0.47%   -57.00     -0.64%
Crypto            12125.75 ±0.86%  12234.00 ±0.43%   108.25      0.89%
Deltablue          7695.00 ±0.24%   7735.27 ±0.69%    40.27      0.52%
Earley-boyer      10561.20 ±0.23%  10753.70 ±0.75%   192.50      1.82%
Gbemu             17752.67 ±0.71%  17980.00 ±0.54%   227.33      1.28%
Mandreel          10558.83 ±0.48%  10547.43 ±0.32%   -11.40     -0.11%
Mandreel latency  29307.17 ±0.87%  29892.50 ±0.94%   585.33      2.00%
Navier-stokes     14999.00 ±0.90%  15046.50 ±0.88%    47.50      0.32%
Pdfjs              7300.00 ±0.33%   7311.83 ±0.20%    11.83      0.16%
Raytrace          15915.00 ±0.13%  16177.00 ±0.95%   262.00      1.65%
Regexp             1529.63 ±0.23%   1690.67 ±0.20%   161.04     10.53%  Improved
Richards           8593.67 ±0.94%   8702.00 ±0.98%   108.33      1.26%
Splay             10626.91 ±0.23%  10881.27 ±0.17%   254.36      2.39%  Improved
Splay latency     22611.63 ±2.99%  24481.75 ±2.04%  1870.12      8.27%  Likely improved
Typescript        16565.17 ±0.15%  16412.17 ±0.81%  -153.00     -0.92%
Zlib              19689.00 ±0.36%  19647.50 ±0.91%   -41.50     -0.21%
----------------  ---------------  ---------------  -------  ---------  ---------------
Total             11065.60 ±0.64%  11311.37 ±0.75%   245.77      2.22%  Likely improved
```

splay wins are probably not real wins and box2d numbers weren't consistent, but consistently better, regexp wins are definitely real.

lib/Backend/BackwardPass.cpp

@@ -1740,36 +1740,36 @@ BackwardPass::ProcessBailOutCopyProps(BailOutInfo * bailOutInfo, BVSparse<JitAre
             StackSym * typeSpecSym = nullptr;
             auto findTypeSpecSym = [&]()
             {
-            if (bailOutInfo->liveLosslessInt32Syms->Test(symId))
-            {
-                // Var version of the sym is not live, use the int32 version
-                int32StackSym = stackSym->GetInt32EquivSym(nullptr);
+                if (bailOutInfo->liveLosslessInt32Syms->Test(symId))
+                {
+                    // Var version of the sym is not live, use the int32 version
+                    int32StackSym = stackSym->GetInt32EquivSym(nullptr);
                     typeSpecSym = int32StackSym;
-                Assert(int32StackSym);
-            }
-            else if(bailOutInfo->liveFloat64Syms->Test(symId))
-            {
-                // Var/int32 version of the sym is not live, use the float64 version
-                float64StackSym = stackSym->GetFloat64EquivSym(nullptr);
+                    Assert(int32StackSym);
+                }
+                else if(bailOutInfo->liveFloat64Syms->Test(symId))
+                {
+                    // Var/int32 version of the sym is not live, use the float64 version
+                    float64StackSym = stackSym->GetFloat64EquivSym(nullptr);
                     typeSpecSym = float64StackSym;
-                Assert(float64StackSym);
-            }
+                    Assert(float64StackSym);
+                }
 #ifdef ENABLE_SIMDJS
-            // SIMD_JS
-            else if (bailOutInfo->liveSimd128F4Syms->Test(symId))
-            {
-                simd128StackSym = stackSym->GetSimd128F4EquivSym(nullptr);
+                // SIMD_JS
+                else if (bailOutInfo->liveSimd128F4Syms->Test(symId))
+                {
+                    simd128StackSym = stackSym->GetSimd128F4EquivSym(nullptr);
                     typeSpecSym = simd128StackSym;
-            }
-            else if (bailOutInfo->liveSimd128I4Syms->Test(symId))
-            {
-                simd128StackSym = stackSym->GetSimd128I4EquivSym(nullptr);
+                }
+                else if (bailOutInfo->liveSimd128I4Syms->Test(symId))
+                {
+                    simd128StackSym = stackSym->GetSimd128I4EquivSym(nullptr);
                     typeSpecSym = simd128StackSym;
-            }
+                }
 #endif
-            else
-            {
-                Assert(bailOutInfo->liveVarSyms->Test(symId));
+                else
+                {
+                    Assert(bailOutInfo->liveVarSyms->Test(symId));
                     typeSpecSym = stackSym;
                 }
             };

lib/Backend/GlobOpt.cpp

@@ -2761,7 +2761,7 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
         }
     }
 
-    if (instr->HasBailOutInfo() && !this->IsLoopPrePass())
+    if (!isHoisted && instr->HasBailOutInfo() && !this->IsLoopPrePass())
     {
         GlobOptBlockData * globOptData = CurrentBlockData();
         globOptData->changedSyms->ClearAll();
@@ -16733,12 +16733,7 @@ GlobOpt::OptHoistInvariant(
         EnsureBailTarget(loop);
 
         // Copy bailout info of loop top.
-        if (instr->ReplaceBailOutInfo(loop->bailOutInfo))
-        {
-            // if the old bailout is deleted, reset capturedvalues cached in block
-            block->globOptData.capturedValues = nullptr;
-            block->globOptData.capturedValuesCandidate = nullptr;
-        }
+        instr->ReplaceBailOutInfo(loop->bailOutInfo);
     }
 
     if(!dst)

lib/Backend/GlobOptBailOut.cpp

@@ -92,119 +92,135 @@ GlobOpt::CaptureValuesIncremental(BasicBlock * block,
 
     FOREACH_BITSET_IN_SPARSEBV(symId, block->globOptData.changedSyms)
     {
-        Sym * sym = hasConstValue ? iterConst.Data().Key() : nullptr;
         Value * val = nullptr;
-        HashBucket<Sym *, Value *> * symIdBucket = nullptr;
 
-        // copy unchanged sym to new capturedValues
-        while (sym && sym->m_id < symId)
+        // First process all unchanged syms with m_id < symId. Then, recapture the current changed sym.
+
+        // copy unchanged const sym to new capturedValues
+        Sym * constSym = hasConstValue ? iterConst.Data().Key() : nullptr;
+        while (constSym && constSym->m_id < symId)
         {
-            Assert(sym->IsStackSym());
-            if (!sym->AsStackSym()->HasArgSlotNum())
+            Assert(constSym->IsStackSym());
+            if (!constSym->AsStackSym()->HasArgSlotNum())
             {
-                bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, sym->AsStackSym(), iterConst.Data().Value());
+                bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, constSym->AsStackSym(), iterConst.Data().Value());
             }
 
             hasConstValue = iterConst.Next();
-            sym = hasConstValue ? iterConst.Data().Key() : nullptr;
+            constSym = hasConstValue ? iterConst.Data().Key() : nullptr;
         }
-        if (sym && sym->m_id == symId)
+        if (constSym && constSym->m_id == symId)
         {
             hasConstValue = iterConst.Next();
         }
-        if (symId != Js::Constants::InvalidSymID)
-        {
-            // recapture changed constant sym
 
-            symIdBucket = block->globOptData.symToValueMap->GetBucket(symId);
-            if (symIdBucket == nullptr)
-            {
-                continue;
-            }
-
-            Sym * symIdSym = symIdBucket->value;
-            Assert(symIdSym->IsStackSym() && (symIdSym->AsStackSym()->HasByteCodeRegSlot() || symIdSym->AsStackSym()->HasArgSlotNum()));
-
-            val =  symIdBucket->element;
-            ValueInfo* valueInfo = val->GetValueInfo();
-
-            if (valueInfo->GetSymStore() != nullptr)
-            {
-                int32 intConstValue;
-                BailoutConstantValue constValue;
-
-                if (valueInfo->TryGetIntConstantValue(&intConstValue))
-                {
-                    constValue.InitIntConstValue(intConstValue);
-                    bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, symIdSym->AsStackSym(), constValue);
-
-                    continue;
-                }
-                else if(valueInfo->IsVarConstant())
-                {
-                    constValue.InitVarConstValue(valueInfo->AsVarConstant()->VarValue());
-                    bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, symIdSym->AsStackSym(), constValue);
-
-                    continue;
-                }
-            }
-            else if (!valueInfo->HasIntConstantValue())
-            {
-                continue;
-            }
-        }
-
-        sym = hasCopyPropSym ? iterCopyPropSym.Data().Key() : nullptr;
-
-        // process unchanged sym, but copy sym might have changed
-        while (sym && sym->m_id < symId)
+        // process unchanged sym; copy-prop sym might have changed
+        Sym * capturedSym = hasCopyPropSym ? iterCopyPropSym.Data().Key() : nullptr;
+        while (capturedSym && capturedSym->m_id < symId)
         {
-            StackSym * copyPropSym = iterCopyPropSym.Data().Value();
+            StackSym * capturedCopyPropSym = iterCopyPropSym.Data().Value();
 
-            Assert(sym->IsStackSym());
+            Assert(capturedSym->IsStackSym());
 
-            if (!block->globOptData.changedSyms->Test(copyPropSym->m_id))
+            if (!block->globOptData.changedSyms->Test(capturedCopyPropSym->m_id))
             {
-                if (!sym->AsStackSym()->HasArgSlotNum())
+                if (!capturedSym->AsStackSym()->HasArgSlotNum())
                 {
-                    bailOutCopySymsIter.InsertNodeBefore(this->func->m_alloc, sym->AsStackSym(), copyPropSym);
+                    bailOutCopySymsIter.InsertNodeBefore(this->func->m_alloc, capturedSym->AsStackSym(), capturedCopyPropSym);
                 }
             }
             else
             {
-                if (!sym->AsStackSym()->HasArgSlotNum())
+                if (!capturedSym->AsStackSym()->HasArgSlotNum())
                 {
-                    val = this->currentBlock->globOptData.FindValue(sym);
+                    val = this->currentBlock->globOptData.FindValue(capturedSym);
                     if (val != nullptr)
                     {
-                        CaptureCopyPropValue(block, sym, val, bailOutCopySymsIter);
+                        CaptureCopyPropValue(block, capturedSym, val, bailOutCopySymsIter);
                     }
                 }
             }
 
             hasCopyPropSym = iterCopyPropSym.Next();
-            sym = hasCopyPropSym ? iterCopyPropSym.Data().Key() : nullptr;
+            capturedSym = hasCopyPropSym ? iterCopyPropSym.Data().Key() : nullptr;
         }
-        if (sym && sym->m_id == symId)
+        if (capturedSym && capturedSym->m_id == symId)
         {
             hasCopyPropSym = iterCopyPropSym.Next();
         }
+
+        // recapture changed sym
+        HashBucket<Sym *, Value *> * symIdBucket = nullptr;
         if (symId != Js::Constants::InvalidSymID)
         {
-            // recapture changed copy prop sym
             symIdBucket = block->globOptData.symToValueMap->GetBucket(symId);
             if (symIdBucket != nullptr)
             {
                 Sym * symIdSym = symIdBucket->value;
-                val = this->currentBlock->globOptData.FindValue(symIdSym);
-                if (val != nullptr)
+                Assert(symIdSym->IsStackSym() && (symIdSym->AsStackSym()->HasByteCodeRegSlot() || symIdSym->AsStackSym()->HasArgSlotNum()));
+
+                val = symIdBucket->element;
+                Assert(val);
+                ValueInfo* valueInfo = val->GetValueInfo();
+
+                if (valueInfo->GetSymStore() != nullptr)
                 {
-                    CaptureCopyPropValue(block, symIdSym, val, bailOutCopySymsIter);
+                    int32 intConstValue;
+                    BailoutConstantValue constValue;
+
+                    if (valueInfo->TryGetIntConstantValue(&intConstValue))
+                    {
+                        constValue.InitIntConstValue(intConstValue);
+                        bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, symIdSym->AsStackSym(), constValue);
+                    }
+                    else if (valueInfo->IsVarConstant())
+                    {
+                        constValue.InitVarConstValue(valueInfo->AsVarConstant()->VarValue());
+                        bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, symIdSym->AsStackSym(), constValue);
+                    }
+                    else
+                    {
+                        CaptureCopyPropValue(block, symIdSym, val, bailOutCopySymsIter);
+                    }
                 }
             }
         }
     }
     NEXT_BITSET_IN_SPARSEBV
+
+    // If, after going over the set of changed syms since the last time we captured values,
+    // there are remaining unprocessed entries in the current captured values set, 
+    // they can simply be copied over to the new bailout info.
+    while (hasConstValue)
+    {
+        Sym * constSym = iterConst.Data().Key();
+        Assert(constSym->IsStackSym());
+        Assert(!block->globOptData.changedSyms->Test(constSym->m_id));
+
+        if (!constSym->AsStackSym()->HasArgSlotNum())
+        {
+            bailOutConstValuesIter.InsertNodeBefore(this->func->m_alloc, constSym->AsStackSym(), iterConst.Data().Value());
+        }
+
+        hasConstValue = iterConst.Next();
+    }
+
+    while (hasCopyPropSym)
+    {
+        Sym * capturedSym = iterCopyPropSym.Data().Key();
+        StackSym * capturedCopyPropSym = iterCopyPropSym.Data().Value();
+
+        Assert(capturedSym->IsStackSym());
+        Assert(!block->globOptData.changedSyms->Test(capturedSym->m_id) &&
+            !block->globOptData.changedSyms->Test(capturedCopyPropSym->m_id));
+
+        if (!capturedSym->AsStackSym()->HasArgSlotNum())
+        {
+            bailOutCopySymsIter.InsertNodeBefore(this->func->m_alloc, capturedSym->AsStackSym(), capturedCopyPropSym);
+        }
+
+        hasCopyPropSym = iterCopyPropSym.Next();
+    }
 }
 
 

lib/Backend/GlobOptBlockData.cpp

@@ -143,7 +143,7 @@ GlobOptBlockData::ReuseBlockData(GlobOptBlockData *fromData)
     this->stackLiteralInitFldDataMap = fromData->stackLiteralInitFldDataMap;
 
     this->changedSyms = fromData->changedSyms;
-    this->changedSyms->ClearAll();
+    this->capturedValues = fromData->capturedValues;
 
     this->OnDataReused(fromData);
 }
@@ -184,6 +184,7 @@ GlobOptBlockData::CopyBlockData(GlobOptBlockData *fromData)
     this->hasCSECandidates = fromData->hasCSECandidates;
 
     this->changedSyms = fromData->changedSyms;
+    this->capturedValues = fromData->capturedValues;
 
     this->stackLiteralInitFldDataMap = fromData->stackLiteralInitFldDataMap;
     this->OnDataReused(fromData);
@@ -365,7 +366,8 @@ void GlobOptBlockData::CloneBlockData(BasicBlock *const toBlockContext, BasicBlo
 
     this->changedSyms = JitAnew(alloc, BVSparse<JitArenaAllocator>, alloc);
     this->changedSyms->Copy(fromData->changedSyms);
-
+    this->capturedValues = fromData->capturedValues;
+    
     Assert(fromData->HasData());
     this->OnDataInitialized(alloc);
 }
@@ -476,10 +478,11 @@ GlobOptBlockData::MergeBlockData(
     this->isTempSrc->And(fromData->isTempSrc);
     this->hasCSECandidates &= fromData->hasCSECandidates;
 
+    this->changedSyms->Or(fromData->changedSyms);
     if (this->capturedValues == nullptr)
     {
         this->capturedValues = fromData->capturedValues;
-        this->changedSyms->Or(fromData->changedSyms);
+        
     }
     else
     {
@@ -488,6 +491,7 @@ GlobOptBlockData::MergeBlockData(
             fromData->capturedValues == nullptr ? nullptr : &fromData->capturedValues->constantValues,
             [&](ConstantStackSymValue * symValueFrom, ConstantStackSymValue * symValueTo)
             {
+                Assert(symValueFrom->Key()->m_id == symValueTo->Key()->m_id);
                 return symValueFrom->Value().IsEqual(symValueTo->Value());
             });
 
@@ -496,12 +500,12 @@ GlobOptBlockData::MergeBlockData(
             fromData->capturedValues == nullptr ? nullptr : &fromData->capturedValues->copyPropSyms,
             [&](CopyPropSyms * copyPropSymFrom, CopyPropSyms * copyPropSymTo)
             {
+                Assert(copyPropSymFrom->Key()->m_id == copyPropSymTo->Key()->m_id);
                 if (copyPropSymFrom->Value()->m_id == copyPropSymTo->Value()->m_id)
                 {
-                    Value * val = fromData->FindValue(copyPropSymFrom->Key());
-                    Value * copyVal = fromData->FindValue(copyPropSymTo->Key());
-                    return (val != nullptr && copyVal != nullptr &&
-                        val->GetValueNumber() == copyVal->GetValueNumber());
+                    Value * fromVal = fromData->FindValue(copyPropSymFrom->Key());
+                    Value * toVal = this->FindValue(copyPropSymFrom->Key());
+                    return fromVal && toVal && fromVal->IsEqualTo(toVal);
                 }
                 return false;
             });

lib/Backend/IR.h

@@ -27,6 +27,8 @@ struct CapturedValues
     SListBase<CopyPropSyms> copyPropSyms;                      // Captured copy prop values during glob opt
     BVSparse<JitArenaAllocator> * argObjSyms;                  // Captured arg object symbols during glob opt
 
+
+    CapturedValues() : argObjSyms(nullptr) {}
     ~CapturedValues()
     {
         // Reset SListBase to be exception safe. Captured values are from GlobOpt->func->alloc

test/Optimizer/capturedValuesBugs.js

@@ -0,0 +1,71 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+if (this.WScript && this.WScript.LoadScriptFile) { // Check for running in ch
+    this.WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+}
+
+function test0(iter) {
+  var dependencies = [];
+  var result;
+  for (var i = 0; i < iter; ++i) {
+    result = (function () {
+      var numberOfArgs = arguments.length;
+      callback = function () {
+        var counter = arguments.length;
+        return counter;
+      };
+      return callback.apply(undefined || this, arguments);
+    }).apply(this, dependencies);
+    dependencies.push(i);
+  }
+  return result;
+}
+
+assert.areEqual(test0(16), 15, "test0 should return 15");
+
+function test1() {
+    var obj0 = { };
+    var b = 1;
+    prop0 = [];
+    for(var i = 0; i < 2; ++i) {
+        for(var j = 0; j < 1; ++j) {
+            obj0.prop1;
+            if(1.1)
+                ++b;
+            else {
+                obj0 = {x:1};
+            }
+        }
+    }
+};
+
+test1();
+test1();
+test1();
+
+function test2() {
+    var obj0 = new Object();
+    var c;
+    var e;
+    
+    c = 32235;
+    
+    e = -25689;
+
+    if((1 - (obj0 <= obj0)) ) {
+    } else {
+        e += 12;
+    }
+
+    c = ((e * -4275 ) * (35822 - (17135 ^ (-1))));
+    return e;
+}
+    
+assert.areEqual(test2(), -25677, "test2 should return -25677");
+assert.areEqual(test2(), -25677, "test2 should return -25677");
+assert.areEqual(test2(), -25677, "test2 should return -25677");
+
+print("passed");

test/Optimizer/rlexe.xml

@@ -1439,4 +1439,9 @@
       <files>fgpeepbug.js</files>
     </default>
   </test>
+  <test>
+    <default>
+      <files>capturedValuesBugs.js</files>
+    </default>
+  </test>
 </regress-exe>