[MERGE #5153 @Penguinwizzard] Hoist speculation masking when possible

Merge pull request #5153 from Penguinwizzard:ponch_opt_clean

This change attempts to move speculation-related masking from the
inside of loops to the out-edges.

The bulk of the work happens in the dead store pass; I took advantage
of the multiple passes on loops to gather data about speculated uses,
and then the final dead store pass handles marking instructions which
we think are safe and storing the data for the Lowerer.

There's a new set of data structures introduced in ClusterList.h; the
purpose of these is to try to get relatively efficient set joins when
dealing with the sets of symbols here. There's a couple of parts that
may be a little tailored for this use case (segmentclusterlist is set
up for symbol clustering patterns and for two-pass runs), but I think
we might find another use for them sometime.

The Lowering code is pretty limited to handling the new opcode, which
has a list of symbols to mask, and expands to instructions that block
speculation on those symbols.

Author: Penguinwizzard

lib/Backend/BackwardPass.cpp

@@ -178,9 +178,16 @@ class BackwardPass
     BVSparse<JitArenaAllocator> * intOverflowDoesNotMatterInRangeBySymId;
     BVSparse<JitArenaAllocator> * candidateSymsRequiredToBeInt;
     BVSparse<JitArenaAllocator> * candidateSymsRequiredToBeLossyInt;
-    StackSym *considerSymAsRealUseInNoImplicitCallUses;
+    StackSym * considerSymAsRealUseInNoImplicitCallUses;
     bool intOverflowCurrentlyMattersInRange;
     bool isCollectionPass;
+    enum class CollectionPassSubPhase
+    {
+        None,
+        FirstPass,
+        SecondPass
+    } collectionPassSubPhase;
+    bool isLoopPrepass;
 
     class FloatSymEquivalenceClass
     {

lib/Backend/BackwardPass.h

@@ -561,11 +561,11 @@ FlowGraph::Build(void)
 
                             // Add edge to finally block, leave block
                             this->AddEdge(currentBlock, this->finallyLabelStack->Top()->GetBasicBlock());
-                            this->AddEdge(currentBlock, leaveBlock);                            
+                            this->AddEdge(currentBlock, leaveBlock);
                         }
                     }
                 }
-            }            
+            }
             else if (instr->m_opcode == Js::OpCode::Finally)
             {
                 AssertOrFailFast(!this->finallyLabelStack->Empty());
@@ -581,7 +581,7 @@ FlowGraph::Build(void)
             block->SetBlockNum(blockNum++);
         } NEXT_BLOCK_ALL;
     }
-    
+
     this->FindLoops();
 
 #if DBG_DUMP
@@ -1739,6 +1739,14 @@ FlowGraph::Destroy(void)
             Region * predRegion = nullptr;
             FOREACH_PREDECESSOR_BLOCK(predBlock, block)
             {
+                BasicBlock* intermediateBlock = block;
+                // Skip blocks inserted for airlock/masking purposes
+                while ((predBlock->isAirLockBlock || predBlock->isAirLockCompensationBlock) && predBlock->GetFirstInstr()->AsLabelInstr()->GetRegion() == region)
+                {
+                    Assert(predBlock->GetPredList()->HasOne());
+                    intermediateBlock = predBlock;
+                    predBlock = predBlock->GetPredList()->Head()->GetPred();
+                }
                 predRegion = predBlock->GetFirstInstr()->AsLabelInstr()->GetRegion();
                 if (predBlock->GetLastInstr() == nullptr)
                 {
@@ -1751,7 +1759,7 @@ FlowGraph::Destroy(void)
                     case Js::OpCode::TryCatch:
                     case Js::OpCode::TryFinally:
                         AssertMsg(region->GetParent() == predRegion, "Bad region prop on entry to try-catch/finally");
-                        if (block->GetFirstInstr() == predBlock->GetLastInstr()->AsBranchInstr()->GetTarget())
+                        if (intermediateBlock->GetFirstInstr() == predBlock->GetLastInstr()->AsBranchInstr()->GetTarget())
                         {
                             if (predBlock->GetLastInstr()->m_opcode == Js::OpCode::TryCatch)
                             {
@@ -2069,7 +2077,7 @@ FlowGraph::UpdateRegionForBlockFromEHPred(BasicBlock * block, bool reassign)
     Assert(region || block->GetPredList()->Count() == 0 || block->firstInstr->AsLabelInstr()->GetRegion());
 
     if (region)
-    { 
+    {
         if (!region->ehBailoutData)
         {
             region->AllocateEHBailoutData(this->func, tryInstr);
@@ -2253,15 +2261,36 @@ FlowGraph::InsertAirlockBlock(FlowEdge * edge)
     BasicBlock * sourceBlock = edge->GetPred();
     BasicBlock * sinkBlock = edge->GetSucc();
 
+    IR::Instr * sourceLastInstr = sourceBlock->GetLastInstr();
+
+    //
+    // Normalize block
+    //
+    if(!sourceLastInstr->IsBranchInstr())
+    {
+        // There are some cases where the last instruction of a block can be not a branch;
+        // for example, if it was previously a conditional branch that was impossible to take.
+        // In these situations, we can insert an unconditional branch to fallthrough for that
+        // block, to renormalize it.
+        SListBaseCounted<FlowEdge*>* successors = sourceBlock->GetSuccList();
+        // Only handling the case for one arc left at the moment; other cases are likely bugs.
+        AssertOrFailFastMsg(successors->HasOne(), "Failed to normalize weird block before airlock");
+        FlowEdge* onlyLink = successors->Head();
+        AssertOrFailFastMsg(onlyLink == edge, "Found duplicate of edge?");
+        AssertOrFailFastMsg(onlyLink->GetSucc() == sinkBlock, "State inconsistent");
+        sourceLastInstr->InsertAfter(IR::BranchInstr::New(Js::OpCode::Br, onlyLink->GetSucc()->GetFirstInstr()->AsLabelInstr(), sourceLastInstr->m_func));
+        sourceLastInstr = sourceLastInstr->m_next;
+    }
+
     BasicBlock * sinkPrevBlock = sinkBlock->prev;
     IR::Instr *  sinkPrevBlockLastInstr = sinkPrevBlock->GetLastInstr();
-    IR::Instr * sourceLastInstr = sourceBlock->GetLastInstr();
 
     airlockBlock->loop = sinkBlock->loop;
     airlockBlock->SetBlockNum(this->blockCount++);
 #ifdef DBG
     airlockBlock->isAirLockBlock = true;
 #endif
+
     //
     // Fixup block linkage
     //
@@ -2312,6 +2341,7 @@ FlowGraph::InsertAirlockBlock(FlowEdge * edge)
     airlockBlock->SetLastInstr(airlockBr);
 
     airlockLabel->SetByteCodeOffset(sinkLabel);
+    airlockLabel->SetRegion(sinkLabel->GetRegion());
 
     // Fixup flow out of sourceBlock
     IR::BranchInstr *sourceBr = sourceLastInstr->AsBranchInstr();
@@ -2433,6 +2463,7 @@ FlowGraph::InsertCompensationCodeForBlockMove(FlowEdge * edge,  bool insertToLoo
     compBlock->SetLastInstr(compBr);
 
     compLabel->SetByteCodeOffset(sinkLabel);
+    compLabel->SetRegion(sinkLabel->GetRegion());
 
     // Fixup flow out of sourceBlock
     if (sourceLastInstr->IsBranchInstr())

lib/Backend/FlowGraph.cpp

@@ -603,6 +603,10 @@ class Loop
     BasicBlock *dominatingLoopCountableBlock;
     LoopCount *loopCount;
     SymIdToStackSymMap *loopCountBasedBoundBaseSyms;
+    typedef SegmentClusterList<SymID, JitArenaAllocator> LoopSymClusterList;
+    LoopSymClusterList *symClusterList;
+    BVSparse<JitArenaAllocator> * internallyDereferencedSyms;
+    SList<IR::ByteCodeUsesInstr*> *outwardSpeculationMaskInstrs;
 
     bool                isDead : 1;
     bool                hasDeadStoreCollectionPass : 1;
@@ -729,6 +733,9 @@ class Loop
         dominatingLoopCountableBlock(nullptr),
         loopCount(nullptr),
         loopCountBasedBoundBaseSyms(nullptr),
+        symClusterList(nullptr),
+        internallyDereferencedSyms(nullptr),
+        outwardSpeculationMaskInstrs(nullptr),
         isDead(false),
         allFieldsKilled(false),
         isLeaf(true),

lib/Backend/FlowGraph.h

@@ -932,7 +932,7 @@ class GlobOpt
     bool                    CheckIfInstrInTypeCheckSeqEmitsTypeCheck(IR::Instr* instr, IR::PropertySymOpnd *opnd);
     template<bool makeChanges>
     bool                    ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd, BasicBlock* block, bool updateExistingValue, bool* emitsTypeCheckOut = nullptr, bool* changesTypeValueOut = nullptr, bool *isObjTypeChecked = nullptr);
-    void                    KillObjectHeaderInlinedTypeSyms(BasicBlock *block, bool isObjTypeSpecialized, SymID symId = (SymID)-1);
+    void                    KillObjectHeaderInlinedTypeSyms(BasicBlock *block, bool isObjTypeSpecialized, SymID symId = SymID_Invalid);
     void                    ValueNumberObjectType(IR::Opnd *dstOpnd, IR::Instr *instr);
     void                    SetSingleTypeOnObjectTypeValue(Value* value, const JITTypeHolder type);
     void                    SetTypeSetOnObjectTypeValue(Value* value, Js::EquivalentTypeSet* typeSet);

lib/Backend/GlobOpt.h

@@ -4532,8 +4532,16 @@ Instr::Dump(IRDumpFlags flags)
             }
         }
 
-        Output::SkipToColumn(20);
-        Output::Print(_u("="));
+        if (this->isSafeToSpeculate)
+        {
+            Output::SkipToColumn(19);
+            Output::Print(_u("<=="));
+        }
+        else
+        {
+            Output::SkipToColumn(20);
+            Output::Print(_u("="));
+        }
     }
 
     PrintOpCodeName();
@@ -4610,21 +4618,22 @@ Instr::Dump(IRDumpFlags flags)
         }
     }
 
-    if (this->IsByteCodeUsesInstr())
+    if (this->IsByteCodeUsesInstr() || this->m_opcode == Js::OpCode::SpeculatedLoadFence)
     {
-        if (this->AsByteCodeUsesInstr()->GetByteCodeUpwardExposedUsed())
+        ByteCodeUsesInstr* tempbcu = static_cast<ByteCodeUsesInstr*>(this);
+        if (tempbcu->GetByteCodeUpwardExposedUsed())
         {
             bool first = true;
-            FOREACH_BITSET_IN_SPARSEBV(id, this->AsByteCodeUsesInstr()->GetByteCodeUpwardExposedUsed())
+            FOREACH_BITSET_IN_SPARSEBV(id, tempbcu->GetByteCodeUpwardExposedUsed())
             {
                 Output::Print(first? _u("s%d") : _u(", s%d"), id);
                 first = false;
             }
             NEXT_BITSET_IN_SPARSEBV;
         }
-        if (this->AsByteCodeUsesInstr()->propertySymUse)
+        if (tempbcu->propertySymUse)
         {
-            Output::Print(_u("  PropSym: %d"), this->AsByteCodeUsesInstr()->propertySymUse->m_id);
+            Output::Print(_u("  PropSym: %d"), tempbcu->propertySymUse->m_id);
         }
     }
 

lib/Backend/IR.cpp

@@ -168,7 +168,8 @@ class Instr
         isCtorCall(false),
         isCallInstrProtectedByNoProfileBailout(false),
         hasSideEffects(false),
-        isNonFastPathFrameDisplay(false)
+        isNonFastPathFrameDisplay(false),
+        isSafeToSpeculate(false)
 #if DBG
         , highlight(0)
 #endif
@@ -219,6 +220,10 @@ class Instr
 
     bool            IsCloned() const { return isCloned; }
     void            SetIsCloned(bool isCloned) { this->isCloned = isCloned; }
+
+    bool            IsSafeToSpeculate() const { return isSafeToSpeculate; }
+    void            SetIsSafeToSpeculate(bool isSafe) { this->isSafeToSpeculate = isSafe; }
+
     bool            HasBailOutInfo() const { return hasBailOutInfo; }
     bool            HasAuxBailOut() const { return hasAuxBailOut; }
     bool            HasTypeCheckBailOut() const;
@@ -503,6 +508,9 @@ class Instr
     // used only for SIMD Ld/St from typed arrays.
     // we keep these here to avoid increase in number of opcodes and to not use ExtendedArgs
     uint8           dataWidth;
+#if DBG
+    WORD            highlight;
+#endif
 
 
     bool            isFsBased : 1; // TEMP : just for BS testing
@@ -526,8 +534,9 @@ class Instr
     bool            hasSideEffects : 1; // The instruction cannot be dead stored
     bool            isNonFastPathFrameDisplay : 1;
 protected:
-    bool            isCloned:1;
-    bool            hasBailOutInfo:1;
+    bool            isCloned : 1;
+    bool            hasBailOutInfo : 1;
+    bool            isSafeToSpeculate : 1;
 
     // Used for aux bail out. We are using same bailOutInfo, just different boolean to hide regular bail out.
     // Refer to ConvertToBailOutInstr implementation for details.
@@ -538,11 +547,6 @@ class Instr
     Opnd *          m_dst;
     Opnd *          m_src1;
     Opnd *          m_src2;
-#if DBG
-    WORD            highlight;
-#endif
-
-
 
     void Init(Js::OpCode opcode, IRKind kind, Func * func);
     IR::Instr *     CloneInstr() const;

lib/Backend/IR.h

@@ -204,7 +204,7 @@ Instr::AsProfiledLabelInstr()
 inline bool
 Instr::IsByteCodeUsesInstr() const
 {
-    return GetKind() == IR::InstrKindByteCodeUses;
+    return GetKind() == IR::InstrKindByteCodeUses && m_opcode == Js::OpCode::ByteCodeUses;
 }
 
 inline ByteCodeUsesInstr *

lib/Backend/IR.inl

@@ -2114,7 +2114,7 @@ void LinearScan::RecordLoopUse(Lifetime *lifetime, RegNum reg)
     // We are trying to avoid the need for compensation at the bottom of the loop if
     // the reg ends up being spilled before it is actually used.
     Loop *curLoop = this->curLoop;
-    SymID symId = (SymID)-1;
+    SymID symId = SymID_Invalid;
 
     if (lifetime)
     {