Reposition first-pass point blocks

Point blocks created in the first pass might not yet have a spot for their
point available to insert them into the block list.  This change:
 1. adds tracking of whether the first pass is completed or not,
 2. initializes the NodeOffsetListArray earlier so it will be available
    when point blocks are craeted in readerPrePass,
 2. during the first pass (and pre-pass), updates point block creation to call
    fgAddNodeMSILOffset to ensure that an appropriate spot will be created
    for it in fgReplaceBranchTargets, and
 3. in replaceFlowGraphNodeUses (which gets called from
    fgReplaceBranchTargets), moves the point node to the appropriate spot.

This change also fixes some blocks that are currently out-of-order:
 1. Create endfinally blocks and unreachable continuation blocks as point
    blocks (so the processing above will pick them up)
 2. Add a PreviousNode parameter to makeFlowGraphNode, use it to keep the
    tail block next to the ret block when we process ret.

This gets first-pass created point blocks in correct MSIL offset order
before we process them in the second pass.  The checking in msilToIR is
updated to assert that the blocks are already in sorted order.

Also fix some stale comments around fgReplaceBranchTargets.

include/Reader/reader.h

@@ -3054,7 +3054,16 @@ public:
                                       uint32_t Element) = 0;
   virtual void insertEHAnnotationNode(IRNode *InsertionPointNode,
                                       IRNode *Node) = 0;
+  /// Construct a new FlowGraphNode
+  ///
+  /// \param TargetOffset  The start MSIL offset for the new node
+  /// \param PreviousNode  The new node will follow \p PreviousNode in the
+  ///                      node list if specified (may be nullptr, in which case
+  ///                      the new node will simply be appended)
+  /// \param Region        EHRegion to apply to the new node
+  /// \returns The new FlowGraphNode
   virtual FlowGraphNode *makeFlowGraphNode(uint32_t TargetOffset,
+                                           FlowGraphNode *PreviousNode,
                                            EHRegion *Region) = 0;
   virtual void markAsEHLabel(IRNode *LabelNode) = 0;
   virtual IRNode *makeTryEndNode(void) = 0;

include/Reader/readerir.h

@@ -636,6 +636,7 @@ public:
     throw NotYetImplementedException("insertEHAnnotationNode");
   };
   FlowGraphNode *makeFlowGraphNode(uint32_t TargetOffset,
+                                   FlowGraphNode *PreviousNode,
                                    EHRegion *Region) override;
   void markAsEHLabel(IRNode *LabelNode) override {
     throw NotYetImplementedException("markAsEHLabel");
@@ -1005,6 +1006,18 @@ private:
   /// \returns          The newly-created successor block
   llvm::BasicBlock *splitCurrentBlock(llvm::TerminatorInst **Goto = nullptr);
 
+  /// \brief Move point blocks preceding \p OldBlock to just before \p NewBlock
+  ///
+  /// Point blocks created during the first pass flow-graph construction are
+  /// inserted before temp blocks created for their point.  Then they are moved
+  /// to the appropriate spot when branches to temp blocks are being rewritten.
+  /// This is the routine invoked during branch rewriting to move them.
+  ///
+  /// \param OldBlock Temporary block whose associated point blocks are to be
+  ///                 fixed up
+  /// \param NewBlock "Real" block that begins at the point in question
+  void movePointBlocks(llvm::BasicBlock *OldBlock, llvm::BasicBlock *NewBlock);
+
   /// Insert one instruction in place of another.
   ///
   /// \param OldInstruction The instruction to be removed.  Must have no uses.
@@ -1305,6 +1318,7 @@ private:
   llvm::BasicBlock *UnreachableContinuationBlock;
   bool KeepGenericContextAlive;
   bool NeedsSecurityObject;
+  bool DoneBuildingFlowGraph;
   llvm::BasicBlock *EntryBlock;
   llvm::Instruction *TempInsertionPoint;
   IRNode *MethodSyncHandle; ///< If the method is synchronized, this is

lib/Reader/reader.cpp

@@ -1919,7 +1919,7 @@ FlowGraphNode *ReaderBase::buildFlowGraph(FlowGraphNode **FgTail) {
 // fgAddNodeMSILOffset
 //
 //  The FlowGraphNodeOffsetList acts as a work list. Each time a
-//  branch is added to the the IR stream a temporary target nodeis
+//  branch is added to the the IR stream a temporary target node is
 //  added to the FlowGraphNodeOffsetList.  After all the branches have
 //  been added the worklist is traversed and each temporary node is
 //  replaced with a real one.
@@ -1969,7 +1969,7 @@ FlowGraphNodeOffsetList *ReaderBase::fgAddNodeMSILOffset(
   NewElement->setOffset(TargetOffset);
 
   if (*Node == nullptr) {
-    *Node = makeFlowGraphNode(TargetOffset,
+    *Node = makeFlowGraphNode(TargetOffset, nullptr,
                               fgGetRegionFromMSILOffset(TargetOffset));
   }
   NewElement->setNode(*Node);
@@ -2292,17 +2292,11 @@ ReaderBase::fgReplaceBranchTarget(uint32_t Offset,
     Start = fgNodeGetStartMSILOffset(Block);
     End = fgNodeGetEndMSILOffset(Block);
 
-    // There are blocks at the top of the flow graph that both Start and
-    // End at offset 0.  These don't correspond to MSIL blocks, though,
-    // so we never want to insert labels in those blocks.
-    // Note that this routine is used to insert labels *before* EH regions
-    // are applied, and that branch targets can only be valid
-    // MSIL offsets, which each have distinct instructions and hence
-    // have non-zero block sizes.  Another special case is the final
-    // block, which may have a label but contain no MSIL due, for example,
-    // to an Endfinally instruction.
-    if ((Offset >= Start && Offset < End) ||
-        (Offset == Start && Offset == End && !NextBlock)) {
+    // Find the MSIL block corresponding to the branch target.  Note that the
+    // test used here precludes selecting a point block as the branch target;
+    // point blocks created in the first pass are only reachable by branches
+    // explicitly made to target them in the first pass.
+    if (Offset >= Start && Offset < End) {
 
       // Branch targets must be at the begining of basic blocks. Thus,
       // if this branch target does not Start the block we must split
@@ -2899,11 +2893,6 @@ void ReaderBase::fgBuildPhase1(FlowGraphNode *Block, uint8_t *ILInput,
     StackOffset = 0;
   }
 
-  NodeOffsetListArraySize =
-      (ILInputSize / FLOW_GRAPH_NODE_LIST_ARRAY_STRIDE) + 1;
-  NodeOffsetListArray = (FlowGraphNodeOffsetList **)getTempMemory(
-      sizeof(FlowGraphNodeOffsetList *) * NodeOffsetListArraySize);
-
   // init stuff prior to loop
   IsShortInstr = false;
   IsConditional = false;
@@ -3127,7 +3116,7 @@ void ReaderBase::fgBuildPhase1(FlowGraphNode *Block, uint8_t *ILInput,
       verifyReturnFlow(CurrentOffset);
       fgNodeSetEndMSILOffset(Block, NextOffset);
       if (NextOffset < ILInputSize) {
-        Block = makeFlowGraphNode(NextOffset, nullptr);
+        Block = makeFlowGraphNode(NextOffset, Block, nullptr);
       }
       break;
 
@@ -7894,6 +7883,13 @@ void ReaderBase::msilToIR(void) {
   FlowGraphNodeWorkList *Worklist;
   FlowGraphNode *FgHead, *FgTail;
 
+  // Initialize the NodeOffsetListArray so it can be used even in the
+  // reader pre-pass
+  NodeOffsetListArraySize =
+      (MethodInfo->ILCodeSize / FLOW_GRAPH_NODE_LIST_ARRAY_STRIDE) + 1;
+  NodeOffsetListArray = (FlowGraphNodeOffsetList **)getTempMemory(
+      sizeof(FlowGraphNodeOffsetList *) * NodeOffsetListArraySize);
+
   // Compiler dependent pre-pass
   readerPrePass(MethodInfo->ILCode, MethodInfo->ILCodeSize);
 
@@ -8009,15 +8005,8 @@ void ReaderBase::msilToIR(void) {
     uint32_t StartOffset = fgNodeGetStartMSILOffset(Block);
     uint32_t EndOffset = fgNodeGetEndMSILOffset(Block);
     if (fgNodeIsVisited(Block)) {
-      if (LastInsertedInOrderBlockEndOffset <= StartOffset) {
-        LastInsertedInOrderBlockEndOffset = EndOffset;
-      } else {
-        // This is a block that's not in MSIL offset order.
-        // Assert that this block doesn't propagate operand stack and
-        // doesn't have any msil and, therefore, can be processed out-of-order.
-        ASSERTNR(!fgNodePropagatesOperandStack(Block));
-        ASSERTNR(StartOffset == EndOffset);
-      }
+      assert(LastInsertedInOrderBlockEndOffset <= StartOffset);
+      LastInsertedInOrderBlockEndOffset = EndOffset;
       FlowGraphMSILOffsetOrder.push_back(Block);
     }
   }

lib/Reader/readerir.cpp

@@ -319,6 +319,7 @@ void GenIR::readerPrePass(uint8_t *Buffer, uint32_t NumBytes) {
   LocalVarCorTypes.resize(NumLocals);
   KeepGenericContextAlive = false;
   NeedsSecurityObject = false;
+  DoneBuildingFlowGraph = false;
   UnreachableContinuationBlock = nullptr;
 
   initParamsAndAutos(MethodSignature);
@@ -1884,9 +1885,12 @@ FlowGraphNode *GenIR::fgGetTailBlock() {
 }
 
 FlowGraphNode *GenIR::makeFlowGraphNode(uint32_t TargetOffset,
+                                        FlowGraphNode *PreviousNode,
                                         EHRegion *Region) {
+  BasicBlock *NextBlock =
+      (PreviousNode == nullptr ? nullptr : PreviousNode->getNextNode());
   FlowGraphNode *Node = (FlowGraphNode *)BasicBlock::Create(
-      *JitContext->LLVMContext, "", Function);
+      *JitContext->LLVMContext, "", Function, NextBlock);
   fgNodeSetStartMSILOffset(Node, TargetOffset);
   return Node;
 }
@@ -2035,21 +2039,14 @@ IRNode *GenIR::fgMakeEndFinally(IRNode *InsertNode, EHRegion *FinallyRegion,
   BasicBlock *TargetBlock = Switch->getParent();
   if (TargetBlock == nullptr) {
     // This is the first endfinally for this finally.  Generate a block to
-    // hold the switch.
-    TargetBlock = BasicBlock::Create(*JitContext->LLVMContext, "endfinally",
-                                     Function, Block->getNextNode());
+    // hold the switch. Use the finally end offset as the switch block's
+    // begin/end.
+    TargetBlock = createPointBlock(FinallyRegion->EndMsilOffset, "endfinally");
     LLVMBuilder->SetInsertPoint(TargetBlock);
 
     // Insert the load of the selector variable and the switch.
     LLVMBuilder->Insert((LoadInst *)Switch->getCondition());
     LLVMBuilder->Insert(Switch);
-
-    // Use the finally end offset as the switch block's begin/end.
-    FlowGraphNode *TargetNode = (FlowGraphNode *)TargetBlock;
-    uint32_t EndOffset = FinallyRegion->EndMsilOffset;
-    fgNodeSetStartMSILOffset(TargetNode, EndOffset);
-    fgNodeSetEndMSILOffset(TargetNode, EndOffset);
-    fgNodeSetPropagatesOperandStack(TargetNode, false);
   }
 
   // Generate and return branch to the block that holds the switch
@@ -2087,9 +2084,25 @@ IRNode *GenIR::fgNodeGetEndInsertIRNode(FlowGraphNode *FgNode) {
   return (IRNode *)InsertInst;
 }
 
+void GenIR::movePointBlocks(BasicBlock *OldBlock, BasicBlock *NewBlock) {
+  BasicBlock *MoveBeforeBlock = NewBlock;
+  uint32_t PointOffset = fgNodeGetStartMSILOffset((FlowGraphNode *)OldBlock);
+  BasicBlock *PointBlock = OldBlock->getPrevNode();
+  BasicBlock *PrevBlock = PointBlock->getPrevNode();
+  while (
+      (PointOffset == fgNodeGetStartMSILOffset((FlowGraphNode *)PointBlock)) &&
+      (PointOffset == fgNodeGetEndMSILOffset((FlowGraphNode *)PointBlock))) {
+    PointBlock->moveBefore(MoveBeforeBlock);
+    MoveBeforeBlock = PointBlock;
+    PointBlock = PrevBlock;
+    PrevBlock = PrevBlock->getPrevNode();
+  }
+}
+
 void GenIR::replaceFlowGraphNodeUses(FlowGraphNode *OldNode,
                                      FlowGraphNode *NewNode) {
   BasicBlock *OldBlock = (BasicBlock *)OldNode;
+  movePointBlocks(OldBlock, NewNode);
   OldBlock->replaceAllUsesWith(NewNode);
   OldBlock->eraseFromParent();
 }
@@ -2210,7 +2223,7 @@ FlowGraphNode *GenIR::fgNodeGetNext(FlowGraphNode *FgNode) {
 
 FlowGraphNode *GenIR::fgPrePhase(FlowGraphNode *Fg) { return FirstMSILBlock; }
 
-void GenIR::fgPostPhase() { return; }
+void GenIR::fgPostPhase() { DoneBuildingFlowGraph = true; }
 
 void GenIR::fgAddLabelToBranchList(IRNode *LabelNode, IRNode *BranchNode) {
   return;
@@ -4655,7 +4668,7 @@ uint32_t GenIR::updateLeaveOffset(EHRegion *Region, uint32_t LeaveOffset,
         // First finally for this function; generate an unreachable block
         // that can be used as the default switch target.
         UnreachableContinuationBlock =
-            BasicBlock::Create(Context, "NullDefault", Function);
+            createPointBlock(MethodInfo->ILCodeSize, "NullDefault");
         new UnreachableInst(Context, UnreachableContinuationBlock);
         fgNodeSetPropagatesOperandStack(
             (FlowGraphNode *)UnreachableContinuationBlock, false);
@@ -4841,6 +4854,22 @@ BasicBlock *GenIR::createPointBlock(uint32_t PointOffset,
   // predecessor.
   fgNodeSetPropagatesOperandStack(PointFlowGraphNode, false);
 
+  if (!DoneBuildingFlowGraph) {
+    // Position this block in the list so that it will get moved to its point
+    // at the end of flow-graph construction.
+    if (PointOffset == MethodInfo->ILCodeSize) {
+      // The point is the end of the function, which is already where this
+      // block is.
+    } else {
+      // Request a split at PointOffset, and move this block before the temp
+      // target so it will get moved after the split is created (in
+      // movePointBlocks).
+      FlowGraphNode *Next = nullptr;
+      fgAddNodeMSILOffset(&Next, PointOffset);
+      Block->moveBefore(Next);
+    }
+  }
+
   return Block;
 }