From fe46b640b78c961ba7e22a0fb0647bf373e07bea Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 20 Apr 2026 12:57:16 -0700 Subject: [PATCH 01/11] JIT: relop simplification during redundant branch opts Suppose we have a dominating branch A (with predicate pA) that shares a successor with a dominated branch B (with predicate pB). can optimize away the comparison done in A. Here we extend that optimization to handle some cases where pB does not imply pA, by forming (AND pB pA) (in VN space) and seeing if we can simplify it to a relop over the same operands as pB, or to a constant. If so, we can remove the comparison in done A but now also must modify the comparison done in B. For example ```if ((x >= 100) && (x <= 100)) S;``` can be simplified to ```if (x == 100) S;``` and ```if ((x >= 100) || (x <= 100)) S;``` can be simplified to ```S;``` As part of this, teach VN how to simplify various combinations of AND/OR/NOT involving relops (there are many cases). Incorporates some of the changes from #83859, but does not try and handle "ignorable" side effects. Fixes #98227. --- src/coreclr/jit/redundantbranchopts.cpp | 60 ++++- src/coreclr/jit/valuenum.cpp | 329 ++++++++++++++++++++++++ 2 files changed, 388 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index fbe16664ab7609..57d94d5d9d8287 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -974,8 +974,60 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) rii.domCmpNormVN = blockPathVN; optRelopImpliesRelop(&rii); + bool canOptimize = rii.canInfer && rii.canInferFromTrue && !rii.reverseSense; - if (!(rii.canInfer && rii.canInferFromTrue && !rii.reverseSense)) + genTreeOps newRelop = GT_NONE; + + if (!canOptimize) + { + if (rii.canInfer) + { + JITDUMP("Can't infer along the path we care about; trying simplification instead\n"); + } + else + { + JITDUMP("Can't infer, trying simplification instead\n"); + } + + // See if we can simplfy the VN for blockPathVN AND domPathVN + // + ValueNum andVN = vnStore->VNForFunc(TYP_INT, VNF_AND, blockPathVN, domPathVN); + VNFuncApp andApp; + VNFuncApp pathApp; + if (vnStore->IsVNRelop(andVN, &andApp) && vnStore->GetVNFunc(blockPathVN, &pathApp)) + { + if (andApp.m_args[0] == pathApp.m_args[0] && andApp.m_args[1] == pathApp.m_args[1]) + { + newRelop = (genTreeOps)andApp.m_func; + } + else if (andApp.m_args[0] == pathApp.m_args[1] && andApp.m_args[1] == pathApp.m_args[0]) + { + vnStore->GetRelatedRelop(andVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap); + vnStore->GetVNFunc(andVN, &andApp); + newRelop = (genTreeOps)andApp.m_func; + } + + JITDUMPEXEC(vnStore->vnDump(this, blockPathVN)); + JITDUMP(" AND"); + JITDUMPEXEC(vnStore->vnDump(this, domPathVN)); + JITDUMP(" ==>"); + JITDUMPEXEC(vnStore->vnDump(this, andVN)); + } + + // Might need to reverse the sense here? + // + if (newRelop != GT_NONE) + { + JITDUMP("; simplified to %s\n", GenTree::OpName(newRelop)); + canOptimize = true; + } + else + { + JITDUMP("; not a relop, cannot simplify\n"); + } + } + + if (!canOptimize) { JITDUMP("failed -- Dominated VN " FMT_VN " does not imply dominating VN " FMT_VN "\n", blockPathVN, domPathVN); @@ -1008,6 +1060,12 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) fgMorphBlockStmt(domBlockProbe, domStmt DEBUGARG(__FUNCTION__), /* allowFGChange */ true, /* invalidateDFSTreeOnFGChange */ false); Metrics.RedundantBranchesEliminated++; + + if (newRelop != GT_NONE) + { + tree->SetOper(newRelop); + fgValueNumberTree(tree); + } madeChanges = true; // We can keep looking if we haven't seen any side effects yet along the path to block. diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 3d76fd6333b04f..0da7d165e9f864 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2574,6 +2574,39 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN) } } } + else if (func == VNFunc(GT_NOT)) + { + VNFuncApp funcApp; + if (GetVNFunc(arg0VN, &funcApp)) + { + // NOT(NOT(x)) ==> x + // + if (funcApp.m_func == VNFunc(GT_NOT)) + { + *resultVN = funcApp.m_args[0]; + } + // NOT(relop(x,y)) ==> Reverse(relop)(x,y) + // + else if (VNFuncIsComparison(funcApp.m_func)) + { + *resultVN = GetRelatedRelop(arg0VN, VN_RELATION_KIND::VRK_Reverse); + } + // NOT(AND(x,y)) ==> OR(NOT(x), NOT(y)) + // + else if (funcApp.m_func == VNFunc(GT_AND)) + { + *resultVN = VNForFunc(typ, VNFunc(GT_OR), VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[0]), + VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[1])); + } + // NOT(OR(x,y)) ==> AND(NOT(x), NOT(y)) + // + else if (funcApp.m_func == VNFunc(GT_OR)) + { + *resultVN = VNForFunc(typ, VNFunc(GT_AND), VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[0]), + VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[1])); + } + } + } // Try to perform constant-folding. // @@ -5048,6 +5081,206 @@ bool ValueNumStore::VNEvalShouldFold(var_types typ, VNFunc func, ValueNum arg0VN return true; } +// Table entry describing when AND/OR of two relops +// with identical operands can be combined into one +// or result in true/false +// +struct RelatedRelopEntry +{ + VNFunc relop0; + VNFunc relop1; + int constantValue; + VNFunc jointRelop; +}; + +// clang-format off + +static RelatedRelopEntry s_relatedRelopTable_AND[] = { + // EQ & ... + {VNFunc(GT_EQ), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_EQ), VNFunc(GT_NE), 0, VNF_COUNT}, + {VNFunc(GT_EQ), VNFunc(GT_LE), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_EQ), VNFunc(GT_LT), 0, VNF_COUNT}, + {VNFunc(GT_EQ), VNFunc(GT_GT), 0, VNF_COUNT}, + {VNFunc(GT_EQ), VNFunc(GT_GE), -1, VNFunc(GT_EQ)}, + + {VNFunc(GT_EQ), VNF_LE_UN, -1, VNFunc(GT_EQ)}, + {VNFunc(GT_EQ), VNF_LT_UN, 0, VNF_COUNT}, + {VNFunc(GT_EQ), VNF_GT_UN, 0, VNF_COUNT}, + {VNFunc(GT_EQ), VNF_GE_UN, -1, VNFunc(GT_EQ)}, + + // NE & ... + {VNFunc(GT_NE), VNFunc(GT_EQ), 0, VNF_COUNT}, + {VNFunc(GT_NE), VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNFunc(GT_LE), -1, VNFunc(GT_LT)}, + {VNFunc(GT_NE), VNFunc(GT_LT), -1, VNFunc(GT_LT)}, + {VNFunc(GT_NE), VNFunc(GT_GT), -1, VNFunc(GT_GT)}, + {VNFunc(GT_NE), VNFunc(GT_GE), -1, VNFunc(GT_GT)}, + + {VNFunc(GT_NE), VNF_LE_UN, -1, VNF_LE_UN}, + {VNFunc(GT_NE), VNF_LT_UN, -1, VNF_LT_UN}, + {VNFunc(GT_NE), VNF_GT_UN, -1, VNF_GT_UN}, + {VNFunc(GT_NE), VNF_GE_UN, -1, VNF_GE_UN}, + + // LE & ... + {VNFunc(GT_LE), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_LE), VNFunc(GT_NE), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LE), VNFunc(GT_LE), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LE), VNFunc(GT_LT), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LE), VNFunc(GT_GT), 0, VNF_COUNT}, + {VNFunc(GT_LE), VNFunc(GT_GE), -1, VNFunc(GT_EQ)}, + + // LT & ... + {VNFunc(GT_LT), VNFunc(GT_EQ), 0, VNF_COUNT}, + {VNFunc(GT_LT), VNFunc(GT_NE), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LT), VNFunc(GT_LE), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LT), VNFunc(GT_LT), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LT), VNFunc(GT_GT), 0, VNF_COUNT}, + {VNFunc(GT_LT), VNFunc(GT_GE), 0, VNF_COUNT}, + + // GT & ... + {VNFunc(GT_GT), VNFunc(GT_EQ), 0, VNF_COUNT}, + {VNFunc(GT_GT), VNFunc(GT_NE), -1, VNFunc(GT_GT)}, + {VNFunc(GT_GT), VNFunc(GT_LE), 0, VNF_COUNT}, + {VNFunc(GT_GT), VNFunc(GT_LT), 0, VNF_COUNT}, + {VNFunc(GT_GT), VNFunc(GT_GT), -1, VNFunc(GT_GT)}, + {VNFunc(GT_GT), VNFunc(GT_GE), -1, VNFunc(GT_GT)}, + + // GE & ... + {VNFunc(GT_GE), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_GE), VNFunc(GT_NE), -1, VNFunc(GT_GT)}, + {VNFunc(GT_GE), VNFunc(GT_LE), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_GE), VNFunc(GT_LT), 0, VNF_COUNT}, + {VNFunc(GT_GE), VNFunc(GT_GT), -1, VNFunc(GT_GT)}, + {VNFunc(GT_GE), VNFunc(GT_GE), -1, VNFunc(GT_GE)}, + + // LEU & ... + {VNF_LE_UN, VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNF_LE_UN, VNFunc(GT_NE), -1, VNF_LE_UN}, + {VNF_LE_UN, VNF_LE_UN, -1, VNF_LE_UN}, + {VNF_LE_UN, VNF_LT_UN, -1, VNF_LT_UN}, + {VNF_LE_UN, VNF_GT_UN, 0, VNF_COUNT}, + {VNF_LE_UN, VNF_GE_UN, -1, VNFunc(GT_EQ)}, + + // LTU & ... + {VNF_LT_UN, VNFunc(GT_EQ), 0, VNF_COUNT}, + {VNF_LT_UN, VNFunc(GT_NE), -1, VNF_LT_UN}, + {VNF_LT_UN, VNF_LE_UN, -1, VNF_LT_UN}, + {VNF_LT_UN, VNF_LT_UN, -1, VNF_LT_UN}, + {VNF_LT_UN, VNF_GT_UN, 0, VNF_COUNT}, + {VNF_LT_UN, VNF_GE_UN, 0, VNF_COUNT}, + + // GTU & ... + {VNF_GT_UN, VNFunc(GT_EQ), 0, VNF_COUNT}, + {VNF_GT_UN, VNFunc(GT_NE), -1, VNF_GT_UN}, + {VNF_GT_UN, VNF_LE_UN, 0, VNF_COUNT}, + {VNF_GT_UN, VNF_LT_UN, 0, VNF_COUNT}, + {VNF_GT_UN, VNF_GT_UN, -1, VNF_GT_UN}, + {VNF_GT_UN, VNF_GE_UN, -1, VNF_GT_UN}, + + // GEU & ... + {VNF_GE_UN, VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNF_GE_UN, VNFunc(GT_NE), -1, VNF_GE_UN}, + {VNF_GE_UN, VNF_LE_UN, -1, VNFunc(GT_EQ)}, + {VNF_GE_UN, VNF_LT_UN, 0, VNF_COUNT}, + {VNF_GE_UN, VNF_GT_UN, -1, VNF_GT_UN}, + {VNF_GE_UN, VNF_GE_UN, -1, VNF_GE_UN}, +}; + +static RelatedRelopEntry s_relatedRelopTable_OR[] = { + // EQ | ... + {VNFunc(GT_EQ), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, + {VNFunc(GT_EQ), VNFunc(GT_NE), 1, VNF_COUNT}, + {VNFunc(GT_EQ), VNFunc(GT_LE), -1, VNFunc(GT_LE)}, + {VNFunc(GT_EQ), VNFunc(GT_LT), -1, VNFunc(GT_LE)}, + {VNFunc(GT_EQ), VNFunc(GT_GT), -1, VNFunc(GT_GE)}, + {VNFunc(GT_EQ), VNFunc(GT_GE), -1, VNFunc(GT_GE)}, + + {VNFunc(GT_EQ), VNF_LE_UN, -1, VNF_LE_UN}, + {VNFunc(GT_EQ), VNF_LT_UN, -1, VNF_LE_UN}, + {VNFunc(GT_EQ), VNF_GT_UN, -1, VNF_GE_UN}, + {VNFunc(GT_EQ), VNF_GE_UN, -1, VNF_GE_UN}, + + // NE | ... + {VNFunc(GT_NE), VNFunc(GT_EQ), 1, VNF_COUNT}, + {VNFunc(GT_NE), VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNFunc(GT_LE), 1, VNF_COUNT}, + {VNFunc(GT_NE), VNFunc(GT_LT), -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNFunc(GT_GT), -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNFunc(GT_GE), 1, VNF_COUNT}, + + {VNFunc(GT_NE), VNF_LE_UN, 1, VNF_COUNT}, + {VNFunc(GT_NE), VNF_LT_UN, -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNF_GT_UN, -1, VNFunc(GT_NE)}, + {VNFunc(GT_NE), VNF_GE_UN, 1, VNF_COUNT}, + + // LE | ... + {VNFunc(GT_LE), VNFunc(GT_EQ), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LE), VNFunc(GT_NE), 1, VNF_COUNT}, + {VNFunc(GT_LE), VNFunc(GT_LE), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LE), VNFunc(GT_LT), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LE), VNFunc(GT_GT), 1, VNF_COUNT}, + {VNFunc(GT_LE), VNFunc(GT_GE), 1, VNF_COUNT}, + + // LT | ... + {VNFunc(GT_LT), VNFunc(GT_EQ), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LT), VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNFunc(GT_LT), VNFunc(GT_LE), -1, VNFunc(GT_LE)}, + {VNFunc(GT_LT), VNFunc(GT_LT), -1, VNFunc(GT_LT)}, + {VNFunc(GT_LT), VNFunc(GT_GT), -1, VNFunc(GT_NE)}, + {VNFunc(GT_LT), VNFunc(GT_GE), 1, VNF_COUNT}, + + // GT | ... + {VNFunc(GT_GT), VNFunc(GT_EQ), -1, VNFunc(GT_GE)}, + {VNFunc(GT_GT), VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNFunc(GT_GT), VNFunc(GT_LE), 1, VNF_COUNT}, + {VNFunc(GT_GT), VNFunc(GT_LT), -1, VNFunc(GT_NE)}, + {VNFunc(GT_GT), VNFunc(GT_GT), -1, VNFunc(GT_GT)}, + {VNFunc(GT_GT), VNFunc(GT_GE), -1, VNFunc(GT_GE)}, + + // GE | ... + {VNFunc(GT_GE), VNFunc(GT_EQ), -1, VNFunc(GT_GE)}, + {VNFunc(GT_GE), VNFunc(GT_NE), 1, VNF_COUNT}, + {VNFunc(GT_GE), VNFunc(GT_LE), 1, VNF_COUNT}, + {VNFunc(GT_GE), VNFunc(GT_LT), 1, VNF_COUNT}, + {VNFunc(GT_GE), VNFunc(GT_GT), -1, VNFunc(GT_GE)}, + {VNFunc(GT_GE), VNFunc(GT_GE), -1, VNFunc(GT_GE)}, + + // LEU | ... + {VNF_LE_UN, VNFunc(GT_EQ), -1, VNF_LE_UN}, + {VNF_LE_UN, VNFunc(GT_NE), 1, VNF_COUNT}, + {VNF_LE_UN, VNF_LE_UN, -1, VNF_LE_UN}, + {VNF_LE_UN, VNF_LT_UN, -1, VNF_LE_UN}, + {VNF_LE_UN, VNF_GT_UN, 1, VNF_COUNT}, + {VNF_LE_UN, VNF_GE_UN, 1, VNF_COUNT}, + + // LTU | ... + {VNF_LT_UN, VNFunc(GT_EQ), -1, VNF_LE_UN}, + {VNF_LT_UN, VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNF_LT_UN, VNF_LE_UN, -1, VNF_LE_UN}, + {VNF_LT_UN, VNF_LT_UN, -1, VNF_LT_UN}, + {VNF_LT_UN, VNF_GT_UN, -1, VNFunc(GT_NE)}, + {VNF_LT_UN, VNF_GE_UN, 1, VNF_COUNT}, + + // GTU | ... + {VNF_GT_UN, VNFunc(GT_EQ), -1, VNF_GE_UN}, + {VNF_GT_UN, VNFunc(GT_NE), -1, VNFunc(GT_NE)}, + {VNF_GT_UN, VNF_LE_UN, 1, VNF_COUNT}, + {VNF_GT_UN, VNF_LT_UN, -1, VNFunc(GT_NE)}, + {VNF_GT_UN, VNF_GT_UN, -1, VNF_GT_UN}, + {VNF_GT_UN, VNF_GE_UN, -1, VNF_GE_UN}, + + // GEU | ... + {VNF_GE_UN, VNFunc(GT_EQ), -1, VNF_GE_UN}, + {VNF_GE_UN, VNFunc(GT_NE), 1, VNF_COUNT}, + {VNF_GE_UN, VNF_LE_UN, 1, VNF_COUNT}, + {VNF_GE_UN, VNF_LT_UN, 1, VNF_COUNT}, + {VNF_GE_UN, VNF_GT_UN, -1, VNF_GE_UN}, + {VNF_GE_UN, VNF_GE_UN, -1, VNF_GE_UN}, +}; + +// clang-format on + //---------------------------------------------------------------------------------------- // EvalUsingMathIdentity // - Attempts to evaluate 'func' by using mathematical identities @@ -5360,7 +5593,55 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN if (arg0VN == arg1VN) { resultVN = arg0VN; + break; + } + + // x | ~x == ~0 + ValueNum arg0VNnot = VNForFunc(typ, VNFunc(GT_NOT), arg0VN); + if (arg0VNnot == arg1VN) + { + resultVN = VNAllBitsForType(typ, 1); + break; } + + // relop1(x,y) | relop2(x,y) ==> relop3(x,y) or 0/1 + // + // eg + // LE(x,y) | NE(x,y) ==> NE(x,y) + // LE(x,y) | GT(x,y) ==> 1 + // + VNFuncApp arg0FN; + if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func)) + { + VNFuncApp arg1FN; + if (GetVNFunc(arg1VN, &arg1FN) && VNFuncIsComparison(arg1FN.m_func)) + { + if ((arg0FN.m_args[0] == arg1FN.m_args[0]) && (arg0FN.m_args[1] == arg1FN.m_args[1])) + { + for (const RelatedRelopEntry& entry : s_relatedRelopTable_OR) + { + if (((entry.relop0 == arg0FN.m_func) && (entry.relop1 == arg1FN.m_func)) || + ((entry.relop0 == arg1FN.m_func) && (entry.relop1 == arg0FN.m_func))) + { + if (entry.constantValue == 1) + { + resultVN = VNOneForType(typ); + } + else if (entry.constantValue == 0) + { + resultVN = VNZeroForType(typ); + } + else + { + resultVN = VNForFunc(typ, entry.jointRelop, arg0FN.m_args[0], arg0FN.m_args[1]); + } + break; + } + } + } + } + } + break; } @@ -5405,7 +5686,55 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN if (arg0VN == arg1VN) { resultVN = arg0VN; + break; + } + + // x & ~x == 0 + ValueNum arg0VNnot = VNForFunc(typ, VNFunc(GT_NOT), arg0VN); + if (arg0VNnot == arg1VN) + { + resultVN = ZeroVN; + break; + } + + // relop1(x,y) & relop2(x,y) ==> relop3(x,y) or 0/1 + // + // eg + // LE(x,y) & NE(x,y) ==> LT(x,y) + // LE(x,y) & GT(x,y) ==> 0 + // + VNFuncApp arg0FN; + if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func)) + { + VNFuncApp arg1FN; + if (GetVNFunc(arg1VN, &arg1FN) && VNFuncIsComparison(arg1FN.m_func)) + { + if ((arg0FN.m_args[0] == arg1FN.m_args[0]) && (arg0FN.m_args[1] == arg1FN.m_args[1])) + { + for (const RelatedRelopEntry& entry : s_relatedRelopTable_AND) + { + if (((entry.relop0 == arg0FN.m_func) && (entry.relop1 == arg1FN.m_func)) || + ((entry.relop0 == arg1FN.m_func) && (entry.relop1 == arg0FN.m_func))) + { + if (entry.constantValue == 1) + { + resultVN = VNOneForType(typ); + } + else if (entry.constantValue == 0) + { + resultVN = VNZeroForType(typ); + } + else + { + resultVN = VNForFunc(typ, entry.jointRelop, arg0FN.m_args[0], arg0FN.m_args[1]); + } + break; + } + } + } + } } + break; } From e4c89c9616f43f5921884873bbc5889720883a02 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 21 Apr 2026 16:18:45 -0700 Subject: [PATCH 02/11] review feedback --- src/coreclr/jit/redundantbranchopts.cpp | 48 ++++++++++++++++++++----- src/coreclr/jit/valuenum.cpp | 48 +++++++++++++++++++++++++ src/coreclr/jit/valuenum.h | 3 ++ 3 files changed, 90 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 57d94d5d9d8287..a051e5e980d0c2 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -797,6 +797,19 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) return false; } + // Exclude floating point compares. + // + VNFuncApp treeApp; + if (!vnStore->GetVNFunc(treeNormVN, &treeApp)) + { + return false; + } + + if (varTypeIsFloating(vnStore->TypeOfVN(treeApp.m_args[0]))) + { + return false; + } + // Skip through chains of empty or side effect free blocks. // Watch for cycles. // @@ -976,7 +989,8 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) optRelopImpliesRelop(&rii); bool canOptimize = rii.canInfer && rii.canInferFromTrue && !rii.reverseSense; - genTreeOps newRelop = GT_NONE; + genTreeOps newRelop = GT_NONE; + bool isUnsigned = false; if (!canOptimize) { @@ -994,17 +1008,18 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) ValueNum andVN = vnStore->VNForFunc(TYP_INT, VNF_AND, blockPathVN, domPathVN); VNFuncApp andApp; VNFuncApp pathApp; + VNFunc newRelopFunc = VNF_NONE; if (vnStore->IsVNRelop(andVN, &andApp) && vnStore->GetVNFunc(blockPathVN, &pathApp)) { if (andApp.m_args[0] == pathApp.m_args[0] && andApp.m_args[1] == pathApp.m_args[1]) { - newRelop = (genTreeOps)andApp.m_func; + newRelopFunc = andApp.m_func; } else if (andApp.m_args[0] == pathApp.m_args[1] && andApp.m_args[1] == pathApp.m_args[0]) { - vnStore->GetRelatedRelop(andVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap); + andVN = vnStore->GetRelatedRelop(andVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap); vnStore->GetVNFunc(andVN, &andApp); - newRelop = (genTreeOps)andApp.m_func; + newRelopFunc = andApp.m_func; } JITDUMPEXEC(vnStore->vnDump(this, blockPathVN)); @@ -1014,12 +1029,18 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) JITDUMPEXEC(vnStore->vnDump(this, andVN)); } - // Might need to reverse the sense here? - // - if (newRelop != GT_NONE) + // TODO-CQ: if the AND simplifies to a constant, we can optimize both the dominating branch + // and the current branch. This is likely rare. + + if (newRelopFunc != VNF_NONE) { - JITDUMP("; simplified to %s\n", GenTree::OpName(newRelop)); - canOptimize = true; + newRelop = vnStore->VNRelopToGenTreeOp(newRelopFunc, isUnsigned); + + if (newRelop != GT_NONE) + { + JITDUMP("; simplified to %s%s\n", GenTree::OpName(newRelop), isUnsigned ? " (unsigned)" : ""); + canOptimize = true; + } } else { @@ -1065,6 +1086,15 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) { tree->SetOper(newRelop); fgValueNumberTree(tree); + + if (isUnsigned) + { + tree->gtFlags |= GTF_UNSIGNED; + } + else + { + tree->gtFlags &= ~GTF_UNSIGNED; + } } madeChanges = true; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0da7d165e9f864..20646674313093 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7172,6 +7172,54 @@ VNFunc ValueNumStore::SwapRelop(VNFunc vnf) return swappedFunc; } +//------------------------------------------------------------------------ +// VNRelopToGenTreeOp: return genTreeOps for a relop VNFunc +// +// Arguments: +// vnf - vnf for original relop +// +// Returns: +// GenTreeOps for the relop, or GT_NONE if the original VNFunc was not a relop. +// +genTreeOps ValueNumStore::VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned) +{ + switch (vnf) + { + case VNF_LT: + isUnsigned = false; + return GT_LT; + case VNF_LE: + isUnsigned = false; + return GT_LE; + case VNF_GE: + isUnsigned = false; + return GT_GE; + case VNF_GT: + isUnsigned = false; + return GT_GT; + case VNF_EQ: + isUnsigned = false; + return GT_EQ; + case VNF_NE: + isUnsigned = false; + return GT_NE; + case VNF_LT_UN: + isUnsigned = true; + return GT_LT; + case VNF_LE_UN: + isUnsigned = true; + return GT_LE; + case VNF_GE_UN: + isUnsigned = true; + return GT_GE; + case VNF_GT_UN: + isUnsigned = true; + return GT_GT; + default: + return GT_NONE; + } +} + //------------------------------------------------------------------------ // GetRelatedRelop: return value number for reversed/swapped comparison // diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 47c7af240ce290..bef6895ffa14a6 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1136,6 +1136,9 @@ class ValueNumStore // Returns true iff the VN represents a relop bool IsVNRelop(ValueNum vn, VNFuncApp* pFuncApp = nullptr); + // Map this VNFunc back to a gen tree op (relops only) + genTreeOps VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned); + enum class VN_RELATION_KIND { VRK_Inferred, // (x ? y) From ba9c7b82b5db5c90da94beb6eafaebae5c957ec5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 09:54:51 -0700 Subject: [PATCH 03/11] reverse relop when needed --- src/coreclr/jit/redundantbranchopts.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index a051e5e980d0c2..6671d2e0446612 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1084,6 +1084,11 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) if (newRelop != GT_NONE) { + if (sharedSuccessor == blockTrueSucc) + { + newRelop = GenTree::ReverseRelop(newRelop); + } + tree->SetOper(newRelop); fgValueNumberTree(tree); From 002891e6b3da76896d6f82345439fad5a161e1a9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 12:21:23 -0700 Subject: [PATCH 04/11] Address review feedback on VN relop simplification - Fix 4 incorrect entries in s_relatedRelopTable_AND for unsigned NE combos: NE & LE_UN, NE & GE_UN (and symmetric) now yield LT_UN/GT_UN rather than LE_UN/GE_UN. The prior entries were wrong when x == y (returning true instead of false). - Guard the relop AND/OR simplification tables against floating-point operands. VNF_*_UN is also used to represent unordered-float relops, and the table identities assume a total order, so they are not sound for NaN/unordered semantics. - Remove the NOT(AND)/NOT(OR) DeMorgan rewrites in VN. GT_NOT constant- folds as bitwise complement (~v), so DeMorgan combined with the NOT(relop) -> Reverse(relop) rewrite was not internally consistent. The rewrites were not needed by any consumer and are removed; the NOT(relop) -> Reverse(relop) rewrite remains, with a comment noting it is only sound in contexts where NOT is applied to a boolean (0/1) value. - Apply GTF_UNSIGNED to the rewritten relop before re-running fgValueNumberTree, since the relop VN depends on the flag (GetVNFuncForNode). - Add a caveat comment on VNRelopToGenTreeOp noting that VNF_*_UN is also used for unordered-float relops; callers propagating isUnsigned into GTF_UNSIGNED must ensure the operands are integral. - Fix typo 'simplfy' -> 'simplify'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/redundantbranchopts.cpp | 8 +++-- src/coreclr/jit/valuenum.cpp | 45 ++++++++++++++----------- src/coreclr/jit/valuenum.h | 8 ++++- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 6671d2e0446612..19aa74a61290db 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1003,7 +1003,7 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) JITDUMP("Can't infer, trying simplification instead\n"); } - // See if we can simplfy the VN for blockPathVN AND domPathVN + // See if we can simplify the VN for blockPathVN AND domPathVN // ValueNum andVN = vnStore->VNForFunc(TYP_INT, VNF_AND, blockPathVN, domPathVN); VNFuncApp andApp; @@ -1090,8 +1090,10 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) } tree->SetOper(newRelop); - fgValueNumberTree(tree); + // Update GTF_UNSIGNED before re-value-numbering: relop VN depends + // on the unsigned flag (see GetVNFuncForNode). + // if (isUnsigned) { tree->gtFlags |= GTF_UNSIGNED; @@ -1100,6 +1102,8 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) { tree->gtFlags &= ~GTF_UNSIGNED; } + + fgValueNumberTree(tree); } madeChanges = true; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 20646674313093..353d94c1b1eeec 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2587,24 +2587,17 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN) } // NOT(relop(x,y)) ==> Reverse(relop)(x,y) // + // Note: GT_NOT in JIT IR is bitwise complement (~v), but this + // rewrite treats it as the logical negation of a boolean-valued + // relop, which produces 0/1. This is only sound in contexts + // where NOT is applied to a value known to be 0/1 (as is the + // case throughout the JIT for relop VNs). For non-relop inputs + // NOT remains an opaque bitwise complement (see below). + // else if (VNFuncIsComparison(funcApp.m_func)) { *resultVN = GetRelatedRelop(arg0VN, VN_RELATION_KIND::VRK_Reverse); } - // NOT(AND(x,y)) ==> OR(NOT(x), NOT(y)) - // - else if (funcApp.m_func == VNFunc(GT_AND)) - { - *resultVN = VNForFunc(typ, VNFunc(GT_OR), VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[0]), - VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[1])); - } - // NOT(OR(x,y)) ==> AND(NOT(x), NOT(y)) - // - else if (funcApp.m_func == VNFunc(GT_OR)) - { - *resultVN = VNForFunc(typ, VNFunc(GT_AND), VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[0]), - VNForFunc(typ, VNFunc(GT_NOT), funcApp.m_args[1])); - } } } @@ -5117,10 +5110,10 @@ static RelatedRelopEntry s_relatedRelopTable_AND[] = { {VNFunc(GT_NE), VNFunc(GT_GT), -1, VNFunc(GT_GT)}, {VNFunc(GT_NE), VNFunc(GT_GE), -1, VNFunc(GT_GT)}, - {VNFunc(GT_NE), VNF_LE_UN, -1, VNF_LE_UN}, + {VNFunc(GT_NE), VNF_LE_UN, -1, VNF_LT_UN}, {VNFunc(GT_NE), VNF_LT_UN, -1, VNF_LT_UN}, {VNFunc(GT_NE), VNF_GT_UN, -1, VNF_GT_UN}, - {VNFunc(GT_NE), VNF_GE_UN, -1, VNF_GE_UN}, + {VNFunc(GT_NE), VNF_GE_UN, -1, VNF_GT_UN}, // LE & ... {VNFunc(GT_LE), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, @@ -5156,7 +5149,7 @@ static RelatedRelopEntry s_relatedRelopTable_AND[] = { // LEU & ... {VNF_LE_UN, VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, - {VNF_LE_UN, VNFunc(GT_NE), -1, VNF_LE_UN}, + {VNF_LE_UN, VNFunc(GT_NE), -1, VNF_LT_UN}, {VNF_LE_UN, VNF_LE_UN, -1, VNF_LE_UN}, {VNF_LE_UN, VNF_LT_UN, -1, VNF_LT_UN}, {VNF_LE_UN, VNF_GT_UN, 0, VNF_COUNT}, @@ -5180,7 +5173,7 @@ static RelatedRelopEntry s_relatedRelopTable_AND[] = { // GEU & ... {VNF_GE_UN, VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, - {VNF_GE_UN, VNFunc(GT_NE), -1, VNF_GE_UN}, + {VNF_GE_UN, VNFunc(GT_NE), -1, VNF_GT_UN}, {VNF_GE_UN, VNF_LE_UN, -1, VNFunc(GT_EQ)}, {VNF_GE_UN, VNF_LT_UN, 0, VNF_COUNT}, {VNF_GE_UN, VNF_GT_UN, -1, VNF_GT_UN}, @@ -5610,8 +5603,14 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // LE(x,y) | NE(x,y) ==> NE(x,y) // LE(x,y) | GT(x,y) ==> 1 // + // The truth table assumes integer (total-order) semantics; the + // VNF_*_UN variants here must be unsigned-integer relops, not + // unordered-float relops (which also use VNF_*_UN). Skip this + // simplification for floating-point operands. + // VNFuncApp arg0FN; - if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func)) + if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func) && + !varTypeIsFloating(TypeOfVN(arg0FN.m_args[0]))) { VNFuncApp arg1FN; if (GetVNFunc(arg1VN, &arg1FN) && VNFuncIsComparison(arg1FN.m_func)) @@ -5703,8 +5702,14 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // LE(x,y) & NE(x,y) ==> LT(x,y) // LE(x,y) & GT(x,y) ==> 0 // + // The truth table assumes integer (total-order) semantics; the + // VNF_*_UN variants here must be unsigned-integer relops, not + // unordered-float relops (which also use VNF_*_UN). Skip this + // simplification for floating-point operands. + // VNFuncApp arg0FN; - if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func)) + if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func) && + !varTypeIsFloating(TypeOfVN(arg0FN.m_args[0]))) { VNFuncApp arg1FN; if (GetVNFunc(arg1VN, &arg1FN) && VNFuncIsComparison(arg1FN.m_func)) diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index bef6895ffa14a6..9607c4c60afd08 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1136,7 +1136,13 @@ class ValueNumStore // Returns true iff the VN represents a relop bool IsVNRelop(ValueNum vn, VNFuncApp* pFuncApp = nullptr); - // Map this VNFunc back to a gen tree op (relops only) + // Map this VNFunc back to a gen tree op (relops only). Returns GT_NONE for + // any non-relop VNFunc. `isUnsigned` is set to true for VNF_*_UN variants. + // + // Note: VNF_*_UN is also used to represent unordered floating-point relops + // (see `GetVNFuncForNode`). Callers that propagate `isUnsigned` into a + // GTF_UNSIGNED flag must ensure the operands are integral; this helper + // cannot distinguish the two cases from a VNFunc alone. genTreeOps VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned); enum class VN_RELATION_KIND From 96cf657cef5408e92056dc7c369c5086a31fc3ea Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 12:25:59 -0700 Subject: [PATCH 05/11] Add RedundantBranchSimplify regression tests Cover the new VN relop simplification path in optRedundantDominatingBranch: - GeLe / GeLeSwapped: motivating case where (a >= k && a <= k) simplifies to (a == k), including the operand-swap variant. - GreaterThanOrEqualZero: exercises the polarity-reversal path (the bug that caused CI failures in optboolsreturn). - NeLeUnsigned / NeGeUnsigned: guard against the unsigned NE & LE_UN / NE & GE_UN table entries producing wrong results when x == y. - FloatGeLe: guard against the simplification firing for floating-point operands, where NaN/unordered semantics invalidate the table identities. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../RedundantBranchSimplify.cs | 151 ++++++++++++++++++ .../RedundantBranchSimplify.csproj | 8 + 2 files changed, 159 insertions(+) create mode 100644 src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs create mode 100644 src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.csproj diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs new file mode 100644 index 00000000000000..77434dcbdf88a3 --- /dev/null +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs @@ -0,0 +1,151 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +// These cases exercise the relop simplification path in redundant branch +// elimination: a dominating relop does not directly imply the dominated +// relop, but AND of the two path predicates simplifies to a single relop +// (or the dominated relop can be reversed/rewritten). +public class RedundantBranchSimplify +{ + // if (a >= 100) { if (a <= 100) return 1; } => inner becomes (a == 100) + [MethodImpl(MethodImplOptions.NoInlining)] + private static int GeLe(int a) + { + if (a >= 100) + { + if (a <= 100) + { + return 1; + } + return 2; + } + return 3; + } + + [Theory] + [InlineData(99, 3)] + [InlineData(100, 1)] + [InlineData(101, 2)] + public static void TestGeLe(int a, int expected) => Assert.Equal(expected, GeLe(a)); + + // Shared successor is the TRUE successor of the dominated block: exercises + // the polarity-reversal path (ReverseRelop when sharedSuccessor == blockTrueSucc). + // Equivalent to `a >= 0` implemented as `a == 0 || a > 0`. + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool GreaterThanOrEqualZero(int a) => a == 0 || a > 0; + + [Theory] + [InlineData(-1, false)] + [InlineData(0, true)] + [InlineData(1, true)] + [InlineData(int.MinValue, false)] + [InlineData(int.MaxValue, true)] + public static void TestGreaterThanOrEqualZero(int a, bool expected) => + Assert.Equal(expected, GreaterThanOrEqualZero(a)); + + // Operand-swapped variant: inner uses (100 <= a) rather than (a >= 100) + // to exercise the swap path in the simplification. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int GeLeSwapped(int a) + { + if (100 <= a) + { + if (a <= 100) + { + return 1; + } + return 2; + } + return 3; + } + + [Theory] + [InlineData(99, 3)] + [InlineData(100, 1)] + [InlineData(101, 2)] + public static void TestGeLeSwapped(int a, int expected) => Assert.Equal(expected, GeLeSwapped(a)); + + // Unsigned NE + LE: a regression guard for the unsigned relop AND table. + // if (a != b) { if (a <= b) return 1; } where LE is unsigned-like (via uint). + // Correct joint: a <_un b. If the simplification incorrectly yielded a <=_un b, + // the a == b path would return the wrong answer. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int NeLeUnsigned(uint a, uint b) + { + if (a != b) + { + if (a <= b) + { + return 1; + } + return 2; + } + return 3; + } + + [Theory] + [InlineData(0u, 0u, 3)] + [InlineData(5u, 5u, 3)] + [InlineData(4u, 5u, 1)] + [InlineData(6u, 5u, 2)] + [InlineData(0u, uint.MaxValue, 1)] + [InlineData(uint.MaxValue, 0u, 2)] + public static void TestNeLeUnsigned(uint a, uint b, int expected) => + Assert.Equal(expected, NeLeUnsigned(a, b)); + + // Unsigned NE + GE analog. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int NeGeUnsigned(uint a, uint b) + { + if (a != b) + { + if (a >= b) + { + return 1; + } + return 2; + } + return 3; + } + + [Theory] + [InlineData(0u, 0u, 3)] + [InlineData(5u, 5u, 3)] + [InlineData(4u, 5u, 2)] + [InlineData(6u, 5u, 1)] + [InlineData(0u, uint.MaxValue, 2)] + [InlineData(uint.MaxValue, 0u, 1)] + public static void TestNeGeUnsigned(uint a, uint b, int expected) => + Assert.Equal(expected, NeGeUnsigned(a, b)); + + // Floating-point guard: NaN must be handled correctly. With NaN, both + // `a >= 0.0` and `a <= 0.0` are false, so the inner should return 2 on the + // NaN path. An unsound simplification (e.g., treating `GE & LE` as `EQ` + // without accounting for unordered semantics) would miscompile this. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int FloatGeLe(double a) + { + if (a >= 0.0) + { + if (a <= 0.0) + { + return 1; + } + return 2; + } + return 3; + } + + [Theory] + [InlineData(-1.0, 3)] + [InlineData(0.0, 1)] + [InlineData(1.0, 2)] + [InlineData(double.NaN, 3)] + [InlineData(double.PositiveInfinity, 2)] + [InlineData(double.NegativeInfinity, 3)] + public static void TestFloatGeLe(double a, int expected) => Assert.Equal(expected, FloatGeLe(a)); +} \ No newline at end of file diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.csproj b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From 94ed5f730c69180ad86ce9bc77127da229603680 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 13:32:22 -0700 Subject: [PATCH 06/11] tighten up comments --- src/coreclr/jit/redundantbranchopts.cpp | 3 +-- src/coreclr/jit/valuenum.cpp | 17 ++--------------- .../RedundantBranch/RedundantBranchSimplify.cs | 15 +-------------- 3 files changed, 4 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 19aa74a61290db..9784210494e3c1 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1091,8 +1091,7 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) tree->SetOper(newRelop); - // Update GTF_UNSIGNED before re-value-numbering: relop VN depends - // on the unsigned flag (see GetVNFuncForNode). + // Update GTF_UNSIGNED before re-value-numbering. // if (isUnsigned) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 353d94c1b1eeec..0e85db2daae94e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2587,13 +2587,6 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN) } // NOT(relop(x,y)) ==> Reverse(relop)(x,y) // - // Note: GT_NOT in JIT IR is bitwise complement (~v), but this - // rewrite treats it as the logical negation of a boolean-valued - // relop, which produces 0/1. This is only sound in contexts - // where NOT is applied to a value known to be 0/1 (as is the - // case throughout the JIT for relop VNs). For non-relop inputs - // NOT remains an opaque bitwise complement (see below). - // else if (VNFuncIsComparison(funcApp.m_func)) { *resultVN = GetRelatedRelop(arg0VN, VN_RELATION_KIND::VRK_Reverse); @@ -5603,10 +5596,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // LE(x,y) | NE(x,y) ==> NE(x,y) // LE(x,y) | GT(x,y) ==> 1 // - // The truth table assumes integer (total-order) semantics; the - // VNF_*_UN variants here must be unsigned-integer relops, not - // unordered-float relops (which also use VNF_*_UN). Skip this - // simplification for floating-point operands. + // for integral comparisons // VNFuncApp arg0FN; if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func) && @@ -5702,10 +5692,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // LE(x,y) & NE(x,y) ==> LT(x,y) // LE(x,y) & GT(x,y) ==> 0 // - // The truth table assumes integer (total-order) semantics; the - // VNF_*_UN variants here must be unsigned-integer relops, not - // unordered-float relops (which also use VNF_*_UN). Skip this - // simplification for floating-point operands. + // for integral comparisons // VNFuncApp arg0FN; if (GetVNFunc(arg0VN, &arg0FN) && VNFuncIsComparison(arg0FN.m_func) && diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs index 77434dcbdf88a3..9e42606938cccf 100644 --- a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs @@ -32,9 +32,6 @@ private static int GeLe(int a) [InlineData(101, 2)] public static void TestGeLe(int a, int expected) => Assert.Equal(expected, GeLe(a)); - // Shared successor is the TRUE successor of the dominated block: exercises - // the polarity-reversal path (ReverseRelop when sharedSuccessor == blockTrueSucc). - // Equivalent to `a >= 0` implemented as `a == 0 || a > 0`. [MethodImpl(MethodImplOptions.NoInlining)] private static bool GreaterThanOrEqualZero(int a) => a == 0 || a > 0; @@ -47,8 +44,6 @@ private static int GeLe(int a) public static void TestGreaterThanOrEqualZero(int a, bool expected) => Assert.Equal(expected, GreaterThanOrEqualZero(a)); - // Operand-swapped variant: inner uses (100 <= a) rather than (a >= 100) - // to exercise the swap path in the simplification. [MethodImpl(MethodImplOptions.NoInlining)] private static int GeLeSwapped(int a) { @@ -69,10 +64,6 @@ private static int GeLeSwapped(int a) [InlineData(101, 2)] public static void TestGeLeSwapped(int a, int expected) => Assert.Equal(expected, GeLeSwapped(a)); - // Unsigned NE + LE: a regression guard for the unsigned relop AND table. - // if (a != b) { if (a <= b) return 1; } where LE is unsigned-like (via uint). - // Correct joint: a <_un b. If the simplification incorrectly yielded a <=_un b, - // the a == b path would return the wrong answer. [MethodImpl(MethodImplOptions.NoInlining)] private static int NeLeUnsigned(uint a, uint b) { @@ -97,7 +88,6 @@ private static int NeLeUnsigned(uint a, uint b) public static void TestNeLeUnsigned(uint a, uint b, int expected) => Assert.Equal(expected, NeLeUnsigned(a, b)); - // Unsigned NE + GE analog. [MethodImpl(MethodImplOptions.NoInlining)] private static int NeGeUnsigned(uint a, uint b) { @@ -122,10 +112,7 @@ private static int NeGeUnsigned(uint a, uint b) public static void TestNeGeUnsigned(uint a, uint b, int expected) => Assert.Equal(expected, NeGeUnsigned(a, b)); - // Floating-point guard: NaN must be handled correctly. With NaN, both - // `a >= 0.0` and `a <= 0.0` are false, so the inner should return 2 on the - // NaN path. An unsound simplification (e.g., treating `GE & LE` as `EQ` - // without accounting for unordered semantics) would miscompile this. + // NaN must be handled correctly. [MethodImpl(MethodImplOptions.NoInlining)] private static int FloatGeLe(double a) { From 1b67e0e1b9bc1ca76ba6d3d803e417064e626c74 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 17:56:30 -0700 Subject: [PATCH 07/11] Use Fact methods for special FP values in RedundantBranchSimplify The XUnitWrapperGenerator that emits the merged JIT.opt test runner does not qualify identifiers like 'NaN' when inlining InlineData arguments, causing CS0103 errors in the generated FullRunner.g.cs. Move the NaN / +Infinity / -Infinity cases out of [InlineData] into separate [Fact] methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../RedundantBranch/RedundantBranchSimplify.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs index 9e42606938cccf..3341a4d9550f26 100644 --- a/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchSimplify.cs @@ -131,8 +131,17 @@ private static int FloatGeLe(double a) [InlineData(-1.0, 3)] [InlineData(0.0, 1)] [InlineData(1.0, 2)] - [InlineData(double.NaN, 3)] - [InlineData(double.PositiveInfinity, 2)] - [InlineData(double.NegativeInfinity, 3)] public static void TestFloatGeLe(double a, int expected) => Assert.Equal(expected, FloatGeLe(a)); + + // Special FP values are passed via separate Fact methods because the + // XUnitWrapperGenerator that emits the merged test runner does not + // qualify identifiers like `NaN` in InlineData arguments. + [Fact] + public static void TestFloatGeLeNaN() => Assert.Equal(3, FloatGeLe(double.NaN)); + + [Fact] + public static void TestFloatGeLePositiveInfinity() => Assert.Equal(2, FloatGeLe(double.PositiveInfinity)); + + [Fact] + public static void TestFloatGeLeNegativeInfinity() => Assert.Equal(3, FloatGeLe(double.NegativeInfinity)); } \ No newline at end of file From b497e85fda1caf2f02741a2ee1837c74c38b783b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Apr 2026 18:48:27 -0700 Subject: [PATCH 08/11] Address further review feedback - Mark s_relatedRelopTable_AND/_OR as static const (they are immutable lookup tables). - Initialize isUnsigned = false up front in VNRelopToGenTreeOp so the out parameter is always set, including when the function returns GT_NONE for a non-relop input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/valuenum.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0e85db2daae94e..2e8e77eb2502ea 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5081,7 +5081,7 @@ struct RelatedRelopEntry // clang-format off -static RelatedRelopEntry s_relatedRelopTable_AND[] = { +static const RelatedRelopEntry s_relatedRelopTable_AND[] = { // EQ & ... {VNFunc(GT_EQ), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, {VNFunc(GT_EQ), VNFunc(GT_NE), 0, VNF_COUNT}, @@ -5173,7 +5173,7 @@ static RelatedRelopEntry s_relatedRelopTable_AND[] = { {VNF_GE_UN, VNF_GE_UN, -1, VNF_GE_UN}, }; -static RelatedRelopEntry s_relatedRelopTable_OR[] = { +static const RelatedRelopEntry s_relatedRelopTable_OR[] = { // EQ | ... {VNFunc(GT_EQ), VNFunc(GT_EQ), -1, VNFunc(GT_EQ)}, {VNFunc(GT_EQ), VNFunc(GT_NE), 1, VNF_COUNT}, @@ -7175,25 +7175,20 @@ VNFunc ValueNumStore::SwapRelop(VNFunc vnf) // genTreeOps ValueNumStore::VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned) { + isUnsigned = false; switch (vnf) { case VNF_LT: - isUnsigned = false; return GT_LT; case VNF_LE: - isUnsigned = false; return GT_LE; case VNF_GE: - isUnsigned = false; return GT_GE; case VNF_GT: - isUnsigned = false; return GT_GT; case VNF_EQ: - isUnsigned = false; return GT_EQ; case VNF_NE: - isUnsigned = false; return GT_NE; case VNF_LT_UN: isUnsigned = true; From 772a9bf9cc779fc95d9d7d50f7f2c7232af469fc Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Apr 2026 07:40:46 -0700 Subject: [PATCH 09/11] Address review nits from EgorBo - optRelopImpliesRelop: verify VN is a comparison before inspecting its operands' type (non-relop VNFuncApps with arity >= 1 would previously slip through). - Use GenTree::SetUnsigned()/ClearUnsigned() helpers instead of direct GTF_UNSIGNED flag manipulation. - Change VNRelopToGenTreeOp's out-parameter from bool& to bool* to match repo convention, and document the parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/redundantbranchopts.cpp | 8 ++++---- src/coreclr/jit/valuenum.cpp | 15 ++++++++------- src/coreclr/jit/valuenum.h | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 9784210494e3c1..d8a46cea75c4f0 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -800,7 +800,7 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) // Exclude floating point compares. // VNFuncApp treeApp; - if (!vnStore->GetVNFunc(treeNormVN, &treeApp)) + if (!vnStore->GetVNFunc(treeNormVN, &treeApp) || !ValueNumStore::VNFuncIsComparison(treeApp.m_func)) { return false; } @@ -1034,7 +1034,7 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) if (newRelopFunc != VNF_NONE) { - newRelop = vnStore->VNRelopToGenTreeOp(newRelopFunc, isUnsigned); + newRelop = vnStore->VNRelopToGenTreeOp(newRelopFunc, &isUnsigned); if (newRelop != GT_NONE) { @@ -1095,11 +1095,11 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) // if (isUnsigned) { - tree->gtFlags |= GTF_UNSIGNED; + tree->SetUnsigned(); } else { - tree->gtFlags &= ~GTF_UNSIGNED; + tree->ClearUnsigned(); } fgValueNumberTree(tree); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 2e8e77eb2502ea..b3f5050fb9cc8b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7168,14 +7168,15 @@ VNFunc ValueNumStore::SwapRelop(VNFunc vnf) // VNRelopToGenTreeOp: return genTreeOps for a relop VNFunc // // Arguments: -// vnf - vnf for original relop +// vnf - vnf for original relop +// isUnsigned - [out] set to true if vnf is an unsigned integer relop, false otherwise // // Returns: // GenTreeOps for the relop, or GT_NONE if the original VNFunc was not a relop. // -genTreeOps ValueNumStore::VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned) +genTreeOps ValueNumStore::VNRelopToGenTreeOp(VNFunc vnf, bool* isUnsigned) { - isUnsigned = false; + *isUnsigned = false; switch (vnf) { case VNF_LT: @@ -7191,16 +7192,16 @@ genTreeOps ValueNumStore::VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned) case VNF_NE: return GT_NE; case VNF_LT_UN: - isUnsigned = true; + *isUnsigned = true; return GT_LT; case VNF_LE_UN: - isUnsigned = true; + *isUnsigned = true; return GT_LE; case VNF_GE_UN: - isUnsigned = true; + *isUnsigned = true; return GT_GE; case VNF_GT_UN: - isUnsigned = true; + *isUnsigned = true; return GT_GT; default: return GT_NONE; diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 9607c4c60afd08..b9d9ec6eff6b82 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -1143,7 +1143,7 @@ class ValueNumStore // (see `GetVNFuncForNode`). Callers that propagate `isUnsigned` into a // GTF_UNSIGNED flag must ensure the operands are integral; this helper // cannot distinguish the two cases from a VNFunc alone. - genTreeOps VNRelopToGenTreeOp(VNFunc vnf, bool& isUnsigned); + genTreeOps VNRelopToGenTreeOp(VNFunc vnf, bool* isUnsigned); enum class VN_RELATION_KIND { From 47facdd47fb4c3508d7c02f210cc26b2c8c7f961 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Apr 2026 08:06:02 -0700 Subject: [PATCH 10/11] Guard x | ~x identity against relop-valued operands With the new NOT(relop) => Reverse(relop) rewrite, computing VNForFunc(GT_NOT, x) for a relop x returns a relop VN rather than an opaque bitwise-NOT node. That broke the x | ~x == AllBitsSet identity for relop operands: e.g. LT(a,b) | GE(a,b) would fold to -1 instead of boolean 1. Skip the identity when arg0 is a relop; the relop-combination table below handles relop | Reverse(relop) and yields VNOneForType. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/valuenum.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index b3f5050fb9cc8b..26ff0c11ff04f8 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5583,11 +5583,21 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN } // x | ~x == ~0 - ValueNum arg0VNnot = VNForFunc(typ, VNFunc(GT_NOT), arg0VN); - if (arg0VNnot == arg1VN) + // + // Skip when x is a relop: relops have boolean 0/1 values, so the + // result should be 1, not AllBitsSet. The relop-combination table + // below handles `relop | Reverse(relop)` and yields VNOneForType. + // + VNFuncApp arg0Check; + const bool arg0IsRelop = GetVNFunc(arg0VN, &arg0Check) && VNFuncIsComparison(arg0Check.m_func); + if (!arg0IsRelop) { - resultVN = VNAllBitsForType(typ, 1); - break; + ValueNum arg0VNnot = VNForFunc(typ, VNFunc(GT_NOT), arg0VN); + if (arg0VNnot == arg1VN) + { + resultVN = VNAllBitsForType(typ, 1); + break; + } } // relop1(x,y) | relop2(x,y) ==> relop3(x,y) or 0/1 From f4b2947b83aef3624fc5445526cf600412391517 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Apr 2026 08:30:57 -0700 Subject: [PATCH 11/11] format --- src/coreclr/jit/valuenum.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 26ff0c11ff04f8..8950270870abc7 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5588,7 +5588,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // result should be 1, not AllBitsSet. The relop-combination table // below handles `relop | Reverse(relop)` and yields VNOneForType. // - VNFuncApp arg0Check; + VNFuncApp arg0Check; const bool arg0IsRelop = GetVNFunc(arg0VN, &arg0Check) && VNFuncIsComparison(arg0Check.m_func); if (!arg0IsRelop) {