From 85b982a28f18610b592b70ac66c592b79994ed25 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 13 May 2022 08:57:15 -0700 Subject: [PATCH] must always check reachability --- src/coreclr/jit/redundantbranchopts.cpp | 42 +++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index c00c1cd5abd47..4f25f4ad54581 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -107,19 +107,30 @@ 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; - ValueNum treeExcVN = ValueNumStore::NoVN; - ValueNum domCmpExcVN = ValueNumStore::NoVN; - unsigned matchCount = 0; - const unsigned matchLimit = 4; + BasicBlock* prevBlock = block; + BasicBlock* domBlock = block->bbIDom; + int relopValue = -1; + unsigned matchCount = 0; + const unsigned matchLimit = 4; if (domBlock == nullptr) { return false; } + // Unpack the tree's VN + // + ValueNum treeNormVN; + ValueNum treeExcVN; + vnStore->VNUnpackExc(tree->GetVN(VNK_Liberal), &treeNormVN, &treeExcVN); + + // If the treeVN is a constant, defer to const prop + // + if (vnStore->IsVNCOnstant(treeNormVN)) + { + return false; + } + const ValueNumStore::VN_RELATION_KIND vnRelations[] = {ValueNumStore::VN_RELATION_KIND::VRK_Same, ValueNumStore::VN_RELATION_KIND::VRK_Reverse, ValueNumStore::VN_RELATION_KIND::VRK_Swap, @@ -143,10 +154,9 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // // Look for an exact match and also try the various swapped/reversed forms. // - ValueNum treeNormVN; - vnStore->VNUnpackExc(tree->GetVN(VNK_Liberal), &treeNormVN, &treeExcVN); ValueNum domCmpNormVN; - vnStore->VNUnpackExc(domCmpTree->GetVN(VNK_Liberal), &domCmpNormVN, &domCmpExcVN); + ValueNum domCmpExcVN; + vnStore->VNUnpackExc(domCmpTree->GetVN(VNK_Liberal), &domCmpNormVN, &domCmpExcVN; ValueNumStore::VN_RELATION_KIND vnRelationMatch = ValueNumStore::VN_RELATION_KIND::VRK_Same; bool matched = false; @@ -289,12 +299,12 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) BasicBlock* const trueSuccessor = domBlock->bbJumpDest; BasicBlock* const falseSuccessor = domBlock->bbNext; - // Only check reachability when we can also infer the value along that path. + // If we can trace the flow from the dominating relop, we can infer its value. // - const bool trueReaches = canInferFromTrue && optReachable(trueSuccessor, block, domBlock); - const bool falseReaches = canInferFromFalse && optReachable(falseSuccessor, block, domBlock); + const bool trueReaches = optReachable(trueSuccessor, block, domBlock); + const bool falseReaches = optReachable(falseSuccessor, block, domBlock); - if (trueReaches && falseReaches) + if (trueReaches && falseReaches && canInferFromTrue && canInferFromFalse) { // Both dominating compare outcomes reach the current block so we can't infer the // value of the relop. @@ -309,7 +319,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) return true; } } - else if (trueReaches) + else if (trueReaches && !falseReaches && canInferFromTrue) { // Taken jump in dominator reaches, fall through doesn't; relop must be true/false. // @@ -319,7 +329,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) relopValue = relopIsTrue ? 1 : 0; break; } - else if (falseReaches) + else if (falseReaches && !trueReaches && canInferFromFalse) { // Fall through from dominator reaches, taken jump doesn't; relop must be false/true. //