From: Eugene Rozenfeld Date: Fri, 26 Apr 2019 23:16:05 +0000 (-0700) Subject: Clean up fgRemoveDeadStore. X-Git-Tag: accepted/tizen/unified/20190813.215958~42^2~382^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=dc389a8a3378972b42200ce9644fb482ec477543;p=platform%2Fupstream%2Fcoreclr.git Clean up fgRemoveDeadStore. Remove duplication and a couple of goto's from `fgRemoveDeadStore`. This part was very confusing: ` /* If this is GT_CATCH_ARG saved to a local var don't bother */ JITDUMP("removing stmt with no side effects\n"); if (asgNode->gtFlags & GTF_ORDER_SIDEEFF) { if (rhsNode->gtOper == GT_CATCH_ARG) { goto EXTRACT_SIDE_EFFECTS; } } ` The `goto` was to the preceding `if` and was useless since the call to `gtExtractSideEffList(rhsNode, &sideEffList);` couldn't possibly find side effects because we already checked that `rhsNode->gtFlags & GTF_SIDE_EFFECT == 0`. --- diff --git a/src/jit/liveness.cpp b/src/jit/liveness.cpp index 9e6eeda..82bd4e6 100644 --- a/src/jit/liveness.cpp +++ b/src/jit/liveness.cpp @@ -2235,150 +2235,95 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, return false; } - /* Test for interior statement */ + // Check for side effects + GenTree* sideEffList = nullptr; + if (rhsNode->gtFlags & GTF_SIDE_EFFECT) + { +#ifdef DEBUG + if (verbose) + { + printf(FMT_BB " - Dead assignment has side effects...\n", compCurBB->bbNum); + gtDispTree(asgNode); + printf("\n"); + } +#endif // DEBUG + // Extract the side effects + if (rhsNode->TypeGet() == TYP_STRUCT) + { + // This is a block assignment. An indirection of the rhs is not considered to + // happen until the assignment, so we will extract the side effects from only + // the address. + if (rhsNode->OperIsIndir()) + { + assert(rhsNode->OperGet() != GT_NULLCHECK); + rhsNode = rhsNode->AsIndir()->Addr(); + } + } + gtExtractSideEffList(rhsNode, &sideEffList); + } + + // Test for interior statement if (asgNode->gtNext == nullptr) { - /* This is a "NORMAL" statement with the - * assignment node hanging from the GT_STMT node */ + // This is a "NORMAL" statement with the assignment node hanging from the GT_STMT node. noway_assert(compCurStmt->gtStmtExpr == asgNode); JITDUMP("top level assign\n"); - /* Check for side effects */ - - if (rhsNode->gtFlags & GTF_SIDE_EFFECT) + if (sideEffList != nullptr) { - EXTRACT_SIDE_EFFECTS: - /* Extract the side effects */ - - GenTree* sideEffList = nullptr; + noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT); #ifdef DEBUG if (verbose) { - printf(FMT_BB " - Dead assignment has side effects...\n", compCurBB->bbNum); - gtDispTree(asgNode); + printf("Extracted side effects list...\n"); + gtDispTree(sideEffList); printf("\n"); } #endif // DEBUG - if (rhsNode->TypeGet() == TYP_STRUCT) - { - // This is a block assignment. An indirection of the rhs is not considered to - // happen until the assignment, so we will extract the side effects from only - // the address. - if (rhsNode->OperIsIndir()) - { - assert(rhsNode->OperGet() != GT_NULLCHECK); - rhsNode = rhsNode->AsIndir()->Addr(); - } - } - gtExtractSideEffList(rhsNode, &sideEffList); - - if (sideEffList) - { - noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT); -#ifdef DEBUG - if (verbose) - { - printf("Extracted side effects list...\n"); - gtDispTree(sideEffList); - printf("\n"); - } -#endif // DEBUG - /* Replace the assignment statement with the list of side effects */ - noway_assert(sideEffList->gtOper != GT_STMT); + // Replace the assignment statement with the list of side effects + noway_assert(sideEffList->gtOper != GT_STMT); - *pTree = compCurStmt->gtStmtExpr = sideEffList; + *pTree = compCurStmt->gtStmtExpr = sideEffList; #ifdef DEBUG - *treeModf = true; + *treeModf = true; #endif // DEBUG - /* Update ordering, costs, FP levels, etc. */ - gtSetStmtInfo(compCurStmt); - - /* Re-link the nodes for this statement */ - fgSetStmtSeq(compCurStmt); + // Update ordering, costs, FP levels, etc. + gtSetStmtInfo(compCurStmt); - // Since the whole statement gets replaced it is safe to - // re-thread and update order. No need to compute costs again. - *pStmtInfoDirty = false; + // Re-link the nodes for this statement + fgSetStmtSeq(compCurStmt); - /* Compute the live set for the new statement */ - *doAgain = true; - return false; - } - else - { - /* No side effects, most likely we forgot to reset some flags */ - fgRemoveStmt(compCurBB, compCurStmt); + // Since the whole statement gets replaced it is safe to + // re-thread and update order. No need to compute costs again. + *pStmtInfoDirty = false; - return true; - } + // Compute the live set for the new statement + *doAgain = true; + return false; } else { - /* If this is GT_CATCH_ARG saved to a local var don't bother */ - JITDUMP("removing stmt with no side effects\n"); - if (asgNode->gtFlags & GTF_ORDER_SIDEEFF) - { - if (rhsNode->gtOper == GT_CATCH_ARG) - { - goto EXTRACT_SIDE_EFFECTS; - } - } - - /* No side effects - remove the whole statement from the block->bbTreeList */ - + // No side effects - remove the whole statement from the block->bbTreeList fgRemoveStmt(compCurBB, compCurStmt); - /* Since we removed it do not process the rest (i.e. RHS) of the statement - * variables in the RHS will not be marked as live, so we get the benefit of - * propagating dead variables up the chain */ - + // Since we removed it do not process the rest (i.e. RHS) of the statement + // variables in the RHS will not be marked as live, so we get the benefit of + // propagating dead variables up the chain return true; } } else { - /* This is an INTERIOR STATEMENT with a dead assignment - remove it */ - + // This is an INTERIOR STATEMENT with a dead assignment - remove it noway_assert(!VarSetOps::IsMember(this, life, varDsc->lvVarIndex)); - if (rhsNode->gtFlags & GTF_SIDE_EFFECT) + if (sideEffList != nullptr) { - /* :-( we have side effects */ - - GenTree* sideEffList = nullptr; -#ifdef DEBUG - if (verbose) - { - printf(FMT_BB " - INTERIOR dead assignment has side effects...\n", compCurBB->bbNum); - gtDispTree(asgNode); - printf("\n"); - } -#endif // DEBUG - - if (rhsNode->TypeGet() == TYP_STRUCT) - { - // This is a block assignment. An indirection of the rhs is not considered to - // happen until the assignment, so we will extract the side effects from only - // the address. - if (rhsNode->OperIsIndir()) - { - assert(rhsNode->OperGet() != GT_NULLCHECK); - rhsNode = rhsNode->AsIndir()->Addr(); - } - } - - gtExtractSideEffList(rhsNode, &sideEffList); - - if (!sideEffList) - { - goto NO_SIDE_EFFECTS; - } - noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT); #ifdef DEBUG if (verbose) @@ -2402,7 +2347,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, #ifdef DEBUG *treeModf = true; #endif // DEBUG - /* Change the node to a GT_COMMA holding the side effect list */ + // Change the node to a GT_COMMA holding the side effect list asgNode->gtBashToNOP(); asgNode->ChangeOper(GT_COMMA); @@ -2422,7 +2367,6 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, } else { - NO_SIDE_EFFECTS: #ifdef DEBUG if (verbose) { @@ -2433,7 +2377,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, printf("\n"); } #endif // DEBUG - /* No side effects - Change the assignment to a GT_NOP node */ + // No side effects - Change the assignment to a GT_NOP node asgNode->gtBashToNOP(); #ifdef DEBUG @@ -2441,7 +2385,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, #endif // DEBUG } - /* Re-link the nodes for this statement - Do not update ordering! */ + // Re-link the nodes for this statement - Do not update ordering! // Do not update costs by calling gtSetStmtInfo. fgSetStmtSeq modifies // the tree threading based on the new costs. Removing nodes could @@ -2452,7 +2396,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, fgSetStmtSeq(compCurStmt); - /* Continue analysis from this node */ + // Continue analysis from this node *pTree = asgNode;