diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index ecd1f6ce603ce..621e395312a76 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -107,11 +107,13 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // Walk up the dom tree and see if any dominating block has branched on // exactly this tree's VN... // - BasicBlock* prevBlock = block; - BasicBlock* domBlock = block->bbIDom; - int relopValue = -1; - unsigned matchCount = 0; - const unsigned matchLimit = 4; + BasicBlock* prevBlock = block; + BasicBlock* domBlock = block->bbIDom; + int relopValue = -1; + ValueNum treeExcVN = ValueNumStore::NoVN; + ValueNum domCmpExcVN = ValueNumStore::NoVN; + unsigned matchCount = 0; + const unsigned matchLimit = 4; if (domBlock == nullptr) { @@ -141,15 +143,17 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // // Look for an exact match and also try the various swapped/reversed forms. // - const ValueNum treeVN = tree->GetVN(VNK_Liberal); - const ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal); + ValueNum treeNormVN; + vnStore->VNUnpackExc(tree->GetVN(VNK_Liberal), &treeNormVN, &treeExcVN); + ValueNum domCmpNormVN; + vnStore->VNUnpackExc(domCmpTree->GetVN(VNK_Liberal), &domCmpNormVN, &domCmpExcVN); ValueNumStore::VN_RELATION_KIND vnRelationMatch = ValueNumStore::VN_RELATION_KIND::VRK_Same; bool matched = false; for (auto vnRelation : vnRelations) { - const ValueNum relatedVN = vnStore->GetRelatedRelop(domCmpVN, vnRelation); - if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == treeVN)) + const ValueNum relatedVN = vnStore->GetRelatedRelop(domCmpNormVN, vnRelation); + if ((relatedVN != ValueNumStore::NoVN) && (relatedVN == treeNormVN)) { vnRelationMatch = vnRelation; matched = true; @@ -258,34 +262,41 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) return false; } - // Bail out if tree is has certain side effects + // Be conservative if there is an exception effect and we're in an EH region + // as we might not model the full extent of EH flow. // - // Note we really shouldn't get here if the tree has non-exception effects, - // as they should have impacted the value number. + if (((tree->gtFlags & GTF_EXCEPT) != 0) && block->hasTryIndex()) + { + JITDUMP("Current relop has exception side effect and is in a try, so we won't optimize\n"); + return false; + } + + // Handle the side effects: for exceptions we can know whether we can drop them using the exception sets. + // Other side effects we always leave around (the unused tree will be appropriately transformed by morph). // + bool keepTreeForSideEffects = false; if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0) { - // Bail if there is a non-exception effect. - // - if ((tree->gtFlags & GTF_SIDE_EFFECT) != GTF_EXCEPT) - { - JITDUMP("Current relop has non-exception side effects, so we won't optimize\n"); - return false; - } + keepTreeForSideEffects = true; - // Be conservative if there is an exception effect and we're in an EH region - // as we might not model the full extent of EH flow. - // - if (block->hasTryIndex()) + if (((tree->gtFlags & GTF_SIDE_EFFECT) == GTF_EXCEPT) && vnStore->VNExcIsSubset(domCmpExcVN, treeExcVN)) { - JITDUMP("Current relop has exception side effect and is in a try, so we won't optimize\n"); - return false; + keepTreeForSideEffects = false; } } - JITDUMP("\nRedundant branch opt in " FMT_BB ":\n", block->bbNum); + if (keepTreeForSideEffects) + { + JITDUMP("Current relop has side effects, keeping it, unused\n"); + GenTree* relopComma = gtNewOperNode(GT_COMMA, TYP_INT, tree, gtNewIconNode(relopValue)); + jumpTree->AsUnOp()->gtOp1 = relopComma; + } + else + { + tree->BashToConst(relopValue); + } - tree->BashToConst(relopValue); + JITDUMP("\nRedundant branch opt in " FMT_BB ":\n", block->bbNum); fgMorphBlockStmt(block, stmt DEBUGARG(__FUNCTION__)); return true; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 373848f7f24f5..633fd346e32b5 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4813,6 +4813,8 @@ bool ValueNumStore::IsVNHandle(ValueNum vn) // ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk) { + assert(vn == VNNormalValue(vn)); + if (vrk == VN_RELATION_KIND::VRK_Same) { return vn; @@ -4823,16 +4825,10 @@ ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk) return NoVN; } - // Pull out any exception set. - // - ValueNum valueVN; - ValueNum excepVN; - VNUnpackExc(vn, &valueVN, &excepVN); - // Verify we have a binary func application // VNFuncApp funcAttr; - if (!GetVNFunc(valueVN, &funcAttr)) + if (!GetVNFunc(vn, &funcAttr)) { return NoVN; } @@ -4932,8 +4928,7 @@ ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk) // Create the resulting VN, swapping arguments if needed. // - ValueNum newVN = VNForFunc(TYP_INT, newFunc, funcAttr.m_args[swap ? 1 : 0], funcAttr.m_args[swap ? 0 : 1]); - ValueNum result = VNWithExc(newVN, excepVN); + ValueNum result = VNForFunc(TYP_INT, newFunc, funcAttr.m_args[swap ? 1 : 0], funcAttr.m_args[swap ? 0 : 1]); return result; }