JIT: build basic block pred lists before morph

src/coreclr/src/jit/compiler.cpp

@@ -4622,23 +4622,16 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags
  }
  EndPhase(PHASE_GS_COOKIE);
 
- /* Compute bbNum, bbRefs and bbPreds */
-
- JITDUMP("\nRenumbering the basic blocks for fgComputePred\n");
+ // GC Poll marking assumes block bbnums match lexical block order,
+ // so make sure this is the case.
  fgRenumberBlocks();
 
- noway_assert(!fgComputePredsDone); // This is the first time full (not cheap) preds will be computed.
- fgComputePreds();
- EndPhase(PHASE_COMPUTE_PREDS);
-
  /* If we need to emit GC Poll calls, mark the blocks that need them now. This is conservative and can
  * be optimized later. */
+
  fgMarkGCPollBlocks();
  EndPhase(PHASE_MARK_GC_POLL_BLOCKS);
 
- /* From this point on the flowgraph information such as bbNum,
- * bbRefs or bbPreds has to be kept updated */
-
  // Compute the block and edge weights
  fgComputeBlockAndEdgeWeights();
  EndPhase(PHASE_COMPUTE_EDGE_WEIGHTS);

src/coreclr/src/jit/compiler.hpp

@@ -2729,13 +2729,31 @@ inline unsigned Compiler::fgThrowHlpBlkStkLevel(BasicBlock* block)
 */
 inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block)
 {
- // If we're converting a BBJ_CALLFINALLY block to a BBJ_THROW block,
+ JITDUMP("Converting " FMT_BB " to BBJ_THROW\n", block->bbNum);
+
+ // Ordering of the following operations matters.
+ // First, note if we are looking at the first block of a call always pair.
+ const bool isCallAlwaysPair = block->isBBCallAlwaysPair();
+
+ // Scrub this block from the pred lists of any successors
+ fgRemoveBlockAsPred(block);
+
+ // Update jump kind after the scrub.
+ block->bbJumpKind = BBJ_THROW;
+
+ // Any block with a throw is rare
+ block->bbSetRunRarely();
+
+ // If we've converted a BBJ_CALLFINALLY block to a BBJ_THROW block,
  // then mark the subsequent BBJ_ALWAYS block as unreferenced.
- if (block->isBBCallAlwaysPair())
+ //
+ // Must do this after we update bbJumpKind of block.
+ if (isCallAlwaysPair)
  {
  BasicBlock* leaveBlk = block->bbNext;
  noway_assert(leaveBlk->bbJumpKind == BBJ_ALWAYS);
 
+ // leaveBlk is now unreachable, so scrub the pred lists.
  leaveBlk->bbFlags &= ~BBF_DONT_REMOVE;
  leaveBlk->bbRefs = 0;
  leaveBlk->bbPreds = nullptr;
@@ -2757,9 +2775,6 @@ inline void Compiler::fgConvertBBToThrowBB(BasicBlock* block)
  }
 #endif // defined(FEATURE_EH_FUNCLETS) && defined(_TARGET_ARM_)
  }
-
- block->bbJumpKind = BBJ_THROW;
- block->bbSetRunRarely(); // any block with a throw is rare
 }
 
 /*****************************************************************************

src/coreclr/src/jit/flowgraph.cpp

@@ -419,23 +419,31 @@ BasicBlock* Compiler::fgNewBasicBlock(BBjumpKinds jumpKind)
  return block;
 }
 
-/*****************************************************************************
- *
- * Ensures that fgFirstBB is a scratch BasicBlock that we have added.
- * This can be used to add initialization code (without worrying
- * about other blocks jumping to it).
- *
- * Callers have to be careful that they do not mess up the order of things
- * added to fgEnsureFirstBBisScratch in a way as to change semantics.
- */
-
+//------------------------------------------------------------------------
+// fgEnsureFirstBBisScratch: Ensure that fgFirstBB is a scratch BasicBlock
+//
+// Returns:
+// Nothing. May allocate a new block and alter the value of fgFirstBB.
+//
+// Notes:
+// This should be called before adding on-entry initialization code to
+// the method, to ensure that fgFirstBB is not part of a loop.
+//
+// Does nothing, if fgFirstBB is already a scratch BB. After calling this,
+// fgFirstBB may already contain code. Callers have to be careful
+// that they do not mess up the order of things added to this block and
+// inadvertently change semantics.
+//
+// We maintain the invariant that a scratch BB ends with BBJ_NONE or
+// BBJ_ALWAYS, so that when adding independent bits of initialization,
+// callers can generally append to the fgFirstBB block without worring
+// about what code is there already.
+//
+// Can be called at any time, and can be called multiple times.
+//
 void Compiler::fgEnsureFirstBBisScratch()
 {
- // This method does not update predecessor lists and so must only be called before they are computed.
- assert(!fgComputePredsDone);
-
  // Have we already allocated a scratch block?
-
  if (fgFirstBBisScratch())
  {
  return;
@@ -453,6 +461,15 @@ void Compiler::fgEnsureFirstBBisScratch()
  block->inheritWeight(fgFirstBB);
  }
 
+ // The first block has an implicit ref count which we must
+ // remove. Note the ref count could be greater that one, if
+ // the first block is not scratch and is targeted by a
+ // branch.
+ assert(fgFirstBB->bbRefs >= 1);
+ fgFirstBB->bbRefs--;
+
+ // The new scratch bb will fall through to the old first bb
+ fgAddRefPred(fgFirstBB, block);
  fgInsertBBbefore(fgFirstBB, block);
  }
  else
@@ -466,6 +483,9 @@ void Compiler::fgEnsureFirstBBisScratch()
 
  block->bbFlags |= (BBF_INTERNAL | BBF_IMPORTED);
 
+ // This new first BB has an implicit ref, and no others.
+ block->bbRefs = 1;
+
  fgFirstBBScratch = fgFirstBB;
 
 #ifdef DEBUG
@@ -476,6 +496,12 @@ void Compiler::fgEnsureFirstBBisScratch()
 #endif
 }
 
+//------------------------------------------------------------------------
+// fgFirstBBisScratch: Check if fgFirstBB is a scratch block
+//
+// Returns:
+// true if fgFirstBB is a scratch block.
+//
 bool Compiler::fgFirstBBisScratch()
 {
  if (fgFirstBBScratch != nullptr)
@@ -497,6 +523,15 @@ bool Compiler::fgFirstBBisScratch()
  }
 }
 
+//------------------------------------------------------------------------
+// fgBBisScratch: Check if a given block is a scratch block.
+//
+// Arguments:
+// block - block in question
+//
+// Returns:
+// true if this block is the first block and is a scratch block.
+//
 bool Compiler::fgBBisScratch(BasicBlock* block)
 {
  return fgFirstBBisScratch() && (block == fgFirstBB);
@@ -8244,7 +8279,7 @@ void Compiler::fgConvertSyncReturnToLeave(BasicBlock* block)
  // Convert the BBJ_RETURN to BBJ_ALWAYS, jumping to genReturnBB.
  block->bbJumpKind = BBJ_ALWAYS;
  block->bbJumpDest = genReturnBB;
- block->bbJumpDest->bbRefs++;
+ fgAddRefPred(genReturnBB, block);
 
 #ifdef DEBUG
  if (verbose)
@@ -8531,7 +8566,7 @@ class MergedReturns
  newReturnBB->bbRefs = 1; // bbRefs gets update later, for now it should be 1
  comp->fgReturnCount++;
 
- newReturnBB->bbFlags |= BBF_INTERNAL;
+ newReturnBB->bbFlags |= (BBF_INTERNAL | BBF_JMP_TARGET);
 
  noway_assert(newReturnBB->bbNext == nullptr);
 
@@ -13102,6 +13137,7 @@ void Compiler::fgComputeBlockAndEdgeWeights()
  const bool usingProfileWeights = fgIsUsingProfileWeights();
  const bool isOptimizing = opts.OptimizationEnabled();
 
+ fgModified = false;
  fgHaveValidEdgeWeights = false;
  fgCalledCount = BB_UNITY_WEIGHT;
 

src/coreclr/src/jit/gentree.cpp

@@ -5483,6 +5483,18 @@ bool GenTree::OperMayThrow(Compiler* comp)
  case GT_ARR_ELEM:
  return comp->fgAddrCouldBeNull(this->AsArrElem()->gtArrObj);
 
+ case GT_FIELD:
+ {
+ GenTree* fldObj = this->AsField()->gtFldObj;
+
+ if (fldObj != nullptr)
+ {
+ return comp->fgAddrCouldBeNull(fldObj);
+ }
+
+ return false;
+ }
+
  case GT_ARR_BOUNDS_CHECK:
  case GT_ARR_INDEX:
  case GT_ARR_OFFSET:

src/coreclr/src/jit/morph.cpp

@@ -7232,6 +7232,11 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
  }
 #endif
 
+ // This block is not longer any block's predecessor. If we end up
+ // converting this tail call to a branch, we'll add appropriate
+ // successor information then.
+ fgRemoveBlockAsPred(compCurBB);
+
 #if !FEATURE_TAILCALL_OPT_SHARED_RETURN
  // We enable shared-ret tail call optimization for recursive calls even if
  // FEATURE_TAILCALL_OPT_SHARED_RETURN is not defined.
@@ -8033,6 +8038,7 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa
  fgFirstBB->bbFlags |= BBF_DONT_REMOVE;
  block->bbJumpKind = BBJ_ALWAYS;
  block->bbJumpDest = fgFirstBB->bbNext;
+ block->bbJumpDest->bbFlags |= BBF_JMP_TARGET;
  fgAddRefPred(block->bbJumpDest, block);
  block->bbFlags &= ~BBF_HAS_JMP;
 }
@@ -14877,12 +14883,6 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
  /* Unconditional throw - transform the basic block into a BBJ_THROW */
  fgConvertBBToThrowBB(block);
 
- /* Remove 'block' from the predecessor list of 'block->bbNext' */
- fgRemoveRefPred(block->bbNext, block);
-
- /* Remove 'block' from the predecessor list of 'block->bbJumpDest' */
- fgRemoveRefPred(block->bbJumpDest, block);
-
 #ifdef DEBUG
  if (verbose)
  {
@@ -15096,19 +15096,6 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
  /* Unconditional throw - transform the basic block into a BBJ_THROW */
  fgConvertBBToThrowBB(block);
 
- /* update the flow graph */
-
- unsigned jumpCnt = block->bbJumpSwt->bbsCount;
- BasicBlock** jumpTab = block->bbJumpSwt->bbsDstTab;
-
- for (unsigned val = 0; val < jumpCnt; val++, jumpTab++)
- {
- BasicBlock* curJump = *jumpTab;
-
- /* Remove 'block' from the predecessor list of 'curJump' */
- fgRemoveRefPred(curJump, block);
- }
-
 #ifdef DEBUG
  if (verbose)
  {
@@ -15207,22 +15194,30 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
  return result;
 }
 
-//*****************************************************************************
+//------------------------------------------------------------------------
+// fgMorphBlockStmt: morph a single statement in a block.
 //
-// Morphs a single statement in a block.
-// Can be called anytime, unlike fgMorphStmts() which should only be called once.
+// Arguments:
+// block - block containing the statement
+// stmt - statement to morph
+// msg - string to identify caller in a dump
 //
-// Returns true if 'stmt' was removed from the block.
-// Returns false if 'stmt' is still in the block (even if other statements were removed).
+// Returns:
+// true if 'stmt' was removed from the block.
+// s false if 'stmt' is still in the block (even if other statements were removed).
+//
+// Notes:
+// Can be called anytime, unlike fgMorphStmts() which should only be called once.
 //
-
 bool Compiler::fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg))
 {
  assert(block != nullptr);
  assert(stmt != nullptr);
 
- compCurBB = block;
- compCurStmt = stmt;
+ // Reset some ambient state
+ fgRemoveRestOfBlock = false;
+ compCurBB = block;
+ compCurStmt = stmt;
 
  GenTree* morph = fgMorphTree(stmt->GetRootNode());
 
@@ -15315,10 +15310,7 @@ bool Compiler::fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(cons
  }
 
  // The rest of block has been removed and we will always throw an exception.
-
- // Update succesors of block
- fgRemoveBlockAsPred(block);
-
+ //
  // For compDbgCode, we prepend an empty BB as the firstBB, it is BBJ_NONE.
  // We should not convert it to a ThrowBB.
  if ((block != fgFirstBB) || ((fgFirstBB->bbFlags & BBF_INTERNAL) == 0))
@@ -15682,6 +15674,7 @@ void Compiler::fgMorphBlocks()
  {
  block->bbJumpKind = BBJ_ALWAYS;
  block->bbJumpDest = genReturnBB;
+ fgAddRefPred(genReturnBB, block);
  fgReturnCount--;
  }
  if (genReturnLocal != BAD_VAR_NUM)
@@ -16642,6 +16635,27 @@ void Compiler::fgMorph()
 
  EndPhase(PHASE_CLONE_FINALLY);
 
+ // Compute bbNum, bbRefs and bbPreds
+ //
+ JITDUMP("\nRenumbering the basic blocks for fgComputePreds\n");
+ fgRenumberBlocks();
+
+ // This is the first time full (not cheap) preds will be computed
+ //
+ noway_assert(!fgComputePredsDone);
+ fgComputePreds();
+
+ // Run an early flow graph simplification pass
+ if (opts.OptimizationEnabled())
+ {
+ fgUpdateFlowGraph();
+ }
+
+ EndPhase(PHASE_COMPUTE_PREDS);
+
+ // From this point on the flowgraph information such as bbNum,
+ // bbRefs or bbPreds has to be kept updated
+
  /* For x64 and ARM64 we need to mark irregular parameters */
  lvaRefCountState = RCS_EARLY;
  fgResetImplicitByRefRefCount();
@@ -18842,18 +18856,34 @@ bool Compiler::fgCheckStmtAfterTailCall()
 
 #if FEATURE_TAILCALL_OPT_SHARED_RETURN
 
- // We can have a move from the call result to an lvaInlineeReturnSpillTemp.
- // However, we can't check that this assignment was created there.
- if (nextMorphStmt->GetRootNode()->gtOper == GT_ASG)
+ // We can have a chain of assignments from the call result to
+ // various inline return spill temps. These are ok as long
+ // as the last one ultimately provides the return value or is ignored.
+ //
+ // And if we're returning a small type we may see a cast
+ // on the source side.
+ while ((nextMorphStmt != nullptr) && (nextMorphStmt->GetRootNode()->OperIs(GT_ASG)))
  {
  Statement* moveStmt = nextMorphStmt;
  GenTree* moveExpr = nextMorphStmt->GetRootNode();
- noway_assert(moveExpr->gtGetOp1()->OperIsLocal() && moveExpr->gtGetOp2()->OperIsLocal());
+ GenTree* moveDest = moveExpr->gtGetOp1();
+ noway_assert(moveDest->OperIsLocal());
+
+ // Tunnel through any casts on the source side.
+ GenTree* moveSource = moveExpr->gtGetOp2();
+ while (moveSource->OperIs(GT_CAST))
+ {
+ noway_assert(!moveSource->gtOverflow());
+ moveSource = moveSource->gtGetOp1();
+ }
+ noway_assert(moveSource->OperIsLocal());
 
- unsigned srcLclNum = moveExpr->gtGetOp2()->AsLclVarCommon()->GetLclNum();
+ // Verify we're just passing the value from one local to another
+ // along the chain.
+ const unsigned srcLclNum = moveSource->AsLclVarCommon()->GetLclNum();
  noway_assert(srcLclNum == callResultLclNumber);
- unsigned dstLclNum  = moveExpr->gtGetOp1()->AsLclVarCommon()->GetLclNum();
- callResultLclNumber = dstLclNum;
+ const unsigned dstLclNum = moveDest->AsLclVarCommon()->GetLclNum();
+ callResultLclNumber  = dstLclNum;
 
  nextMorphStmt = moveStmt->GetNextStmt();
  }