JIT: build basic block pred lists before morph (#1309)

Build basic block pred lists before morph, instead of after, and add an early
flow optimization pass. Fix up a few places where ref counts or pred lists
were not properly maintained in morph.

The early flow opt pass enhances the local assertion prop run in morph (by
fusing blocks), allows the jit to avoid morphing some unreachable blocks
(thus saving a bit of TP), and lays the groundwork for more aggressive early
branch folding that would be useful (eg #27935).

src/coreclr/src/jit/compiler.cpp

@@ -4299,23 +4299,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

@@ -7231,6 +7231,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.
@@ -8032,6 +8037,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;
 }
@@ -14884,12 +14890,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)
  {
@@ -15103,19 +15103,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)
  {
@@ -15214,22 +15201,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());
 
@@ -15322,10 +15317,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))
@@ -15689,6 +15681,7 @@ void Compiler::fgMorphBlocks()
  {
  block->bbJumpKind = BBJ_ALWAYS;
  block->bbJumpDest = genReturnBB;
+ fgAddRefPred(genReturnBB, block);
  fgReturnCount--;
  }
  if (genReturnLocal != BAD_VAR_NUM)
@@ -16649,6 +16642,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();
@@ -17934,18 +17948,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();
  }