PRE fixes - compensation code and avoiding cleanup for PRE instrs' dsts, hoisting LdStr

Author: rajatd

lib/Backend/FlowGraph.cpp

@@ -4227,8 +4227,9 @@ BasicBlock::CleanUpValueMaps()
     {
         FOREACH_SLISTBASE_ENTRY_EDITING(GlobHashBucket, bucket, &thisTable->table[i], iter)
         {
-            bool isSymUpwardExposed = upwardExposedUses->Test(bucket.value->m_id) || upwardExposedFields->Test(bucket.value->m_id);
-            if (!isSymUpwardExposed && symsInCallSequence.Test(bucket.value->m_id))
+            Sym * sym = bucket.value;
+            bool isSymUpwardExposed = upwardExposedUses->Test(sym->m_id) || upwardExposedFields->Test(sym->m_id);
+            if (!isSymUpwardExposed && symsInCallSequence.Test(sym->m_id))
             {
                 // Don't remove/shrink sym-value pair if the sym is referenced in callSequence even if the sym is dead according to backward data flow.
                 // This is possible in some edge cases that an infinite loop is involved when evaluating parameter for a function (between StartCall and Call),
@@ -4241,22 +4242,22 @@ BasicBlock::CleanUpValueMaps()
             // Make sure symbol was created before backward pass.
             // If symbols isn't upward exposed, mark it as dead.
             // If a symbol was copy-prop'd in a loop prepass, the upwardExposedUses info could be wrong.  So wait until we are out of the loop before clearing it.
-            if ((SymID)bucket.value->m_id <= this->globOptData.globOpt->maxInitialSymID && !isSymUpwardExposed
-                && (!isInLoop || !this->globOptData.globOpt->prePassCopyPropSym->Test(bucket.value->m_id)))
+            bool isSymFieldPRESymStore = isInLoop && this->loop->fieldPRESymStores->Test(sym->m_id);
+            if ((SymID)sym->m_id <= this->globOptData.globOpt->maxInitialSymID && !isSymUpwardExposed && !isSymFieldPRESymStore
+                && (!isInLoop || !this->globOptData.globOpt->prePassCopyPropSym->Test(sym->m_id)))
             {
                 Value *val = bucket.element;
                 ValueInfo *valueInfo = val->GetValueInfo();
 
-                Sym * sym = bucket.value;
                 Sym *symStore = valueInfo->GetSymStore();
 
-                if (symStore && symStore == bucket.value)
+                if (symStore && symStore == sym)
                 {
                     // Keep constants around, as we don't know if there will be further uses
                     if (!bucket.element->GetValueInfo()->IsVarConstant() && !bucket.element->GetValueInfo()->HasIntConstantValue())
                     {
                         // Symbol may still be a copy-prop candidate.  Wait before deleting it.
-                        deadSymsBv.Set(bucket.value->m_id);
+                        deadSymsBv.Set(sym->m_id);
 
                         // Make sure the type sym is added to the dead syms vector as well, because type syms are
                         // created in backward pass and so their symIds > maxInitialSymID.
@@ -4287,8 +4288,6 @@ BasicBlock::CleanUpValueMaps()
             }
             else
             {
-                Sym * sym = bucket.value;
-
                 if (sym->IsPropertySym() && !this->globOptData.liveFields->Test(sym->m_id))
                 {
                     // Remove propertySyms which are not live anymore.
@@ -4306,7 +4305,7 @@ BasicBlock::CleanUpValueMaps()
 
                     Sym *symStore = valueInfo->GetSymStore();
 
-                    if (symStore && symStore != bucket.value)
+                    if (symStore && symStore != sym)
                     {
                         keepAliveSymsBv.Set(symStore->m_id);
                         if (symStore->IsStackSym() && symStore->AsStackSym()->HasObjectTypeSym())

lib/Backend/FlowGraph.h

@@ -591,7 +591,7 @@ class Loop
     ValueNumber         firstValueNumberInLoop;
     JsArrayKills        jsArrayKills;
     BVSparse<JitArenaAllocator> *fieldKilled;
-    BVSparse<JitArenaAllocator> *fieldPRESymStore;
+    BVSparse<JitArenaAllocator> *fieldPRESymStores;
     InitialValueFieldMap initialValueFieldMap;
 
     InductionVariableSet *inductionVariables;

lib/Backend/GlobOpt.cpp

@@ -450,6 +450,11 @@ GlobOpt::OptBlock(BasicBlock *block)
         if (loop != this->prePassLoop)
         {
             OptLoops(loop);
+            if (!IsLoopPrePass() && loop->parent)
+            {
+                loop->fieldPRESymStores->Or(loop->parent->fieldPRESymStores);
+            }
+            
             if (!this->IsLoopPrePass() && DoFieldPRE(loop))
             {
                 // Note: !IsLoopPrePass means this was a root loop pre-pass. FieldPre() is called once per loop.
@@ -543,6 +548,7 @@ GlobOpt::OptBlock(BasicBlock *block)
                 if (succ->isLoopHeader && succ->loop->IsDescendentOrSelf(block->loop))
                 {
                     BVSparse<JitArenaAllocator> *liveOnBackEdge = block->loop->regAlloc.liveOnBackEdgeSyms;
+                    liveOnBackEdge->Or(block->loop->fieldPRESymStores);
 
                     this->tempBv->Minus(block->loop->varSymsOnEntry, block->globOptData.liveVarSyms);
                     this->tempBv->And(liveOnBackEdge);
@@ -663,7 +669,7 @@ GlobOpt::OptLoops(Loop *loop)
 
         loop->symsDefInLoop = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
         loop->fieldKilled = JitAnew(alloc, BVSparse<JitArenaAllocator>, this->alloc);
-        loop->fieldPRESymStore = JitAnew(alloc, BVSparse<JitArenaAllocator>, this->alloc);
+        loop->fieldPRESymStores = JitAnew(alloc, BVSparse<JitArenaAllocator>, this->alloc);
         loop->allFieldsKilled = false;
     }
     else
@@ -1035,13 +1041,13 @@ BOOL GlobOpt::PreloadPRECandidate(Loop *loop, GlobHashBucket* candidate)
     if (ldInstr->GetDst()->AsRegOpnd()->m_sym != symStore)
     {
         ldInstr->ReplaceDst(IR::RegOpnd::New(symStore->AsStackSym(), TyVar, this->func));
+        loop->fieldPRESymStores->Set(symStore->m_id);
     }
 
     ldInstr->GetSrc1()->SetIsJITOptimizedReg(true);
     ldInstr->GetDst()->SetIsJITOptimizedReg(true);
 
     landingPad->globOptData.liveVarSyms->Set(symStore->m_id);
-    loop->fieldPRESymStore->Set(symStore->m_id);
 
     ValueType valueType(ValueType::Uninitialized);
     Value *initialValue = nullptr;
@@ -3027,11 +3033,10 @@ GlobOpt::SetLoopFieldInitialValue(Loop *loop, IR::Instr *instr, PropertySym *pro
     Value *initialValue = nullptr;
     StackSym *symStore;
 
-    if (loop->allFieldsKilled || loop->fieldKilled->Test(originalPropertySym->m_id))
+    if (loop->allFieldsKilled || loop->fieldKilled->Test(originalPropertySym->m_id) || loop->fieldKilled->Test(propertySym->m_id))
     {
         return;
     }
-    Assert(!loop->fieldKilled->Test(propertySym->m_id));
 
     // Value already exists
     if (CurrentBlockData()->FindValue(propertySym))
@@ -14156,7 +14161,6 @@ GlobOpt::OptIsInvariant(
         return false;
 
         // Usually not worth hoisting these
-    case Js::OpCode::LdStr:
     case Js::OpCode::Ld_A:
     case Js::OpCode::Ld_I4:
     case Js::OpCode::LdC_A_I4: