More cleanups in rangecheck/assertionprop (#129325)

* Remove `GTF_CHK_INDEX_INBND` and unconditionally always remove
`GT_BOUNDS_CHECK` in assertprop when we can
* Remove `FEATURE_ENABLE_NO_RANGE_CHECKS` and `DOTNET_JitNoRngChks`,
these were never enabled by default and I don't think they have any
value.
* Correctness improvement: we no longer use this [potential
bug](https://github.com/dotnet/runtime/blob/d71e0a994980fc47a1efb4ce2c207947e7efedca/src/coreclr/jit/optimizer.cpp#L5545-L5553)
and extract all side-effects.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: EgorBo

src/coreclr/inc/switches.h

@@ -119,10 +119,6 @@
 
 #endif // _DEBUG
 
-// MUST NEVER CHECK IN WITH THIS ENABLED.
-// This is just for convenience in doing performance investigations in a checked-out enlistment.
-// #define FEATURE_ENABLE_NO_RANGE_CHECKS
-
 // This controls whether a compilation-timing feature that relies on Windows APIs, if available, else direct
 // hardware instructions (rdtsc), for accessing high-resolution hardware timers is enabled. This is disabled
 // in Silverlight (just to avoid thinking about whether the extra code space is worthwhile).

src/coreclr/jit/assertionprop.cpp

@@ -4998,23 +4998,6 @@ GenTree* Compiler::optAssertionProp_Cast(ASSERT_VALARG_TP assertions,
     return nullptr;
 }
 
-/*****************************************************************************
- *
- *  Given a tree with an array bounds check node, eliminate it because it was
- *  checked already in the program.
- */
-GenTree* Compiler::optAssertionProp_Comma(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt)
-{
-    // Remove the bounds check as part of the GT_COMMA node since we need parent pointer to remove nodes.
-    // When processing visits the bounds check, it sets the throw kind to None if the check is redundant.
-    if (tree->gtGetOp1()->OperIs(GT_BOUNDS_CHECK) && ((tree->gtGetOp1()->gtFlags & GTF_CHK_INDEX_INBND) != 0))
-    {
-        optRemoveCommaBasedRangeCheck(tree, stmt);
-        return optAssertionProp_Update(tree, tree, stmt);
-    }
-    return nullptr;
-}
-
 //------------------------------------------------------------------------
 // optAssertionProp_Ind: see if we can prove the indirection can't cause
 //    and exception.
@@ -5501,59 +5484,40 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions,
                                             Statement*       stmt,
                                             BasicBlock*      block)
 {
-    if (optLocalAssertionProp)
-    {
-        return nullptr;
-    }
-
     assert(tree->OperIs(GT_BOUNDS_CHECK));
-
-#ifdef FEATURE_ENABLE_NO_RANGE_CHECKS
-    if (JitConfig.JitNoRngChks())
+    if (optLocalAssertionProp)
     {
-#ifdef DEBUG
-        if (verbose)
-        {
-            printf("\nFlagging check redundant due to JitNoRngChks in " FMT_BB ":\n", compCurBB->bbNum);
-            gtDispTree(tree, nullptr, nullptr, true);
-        }
-#endif // DEBUG
-        tree->gtFlags |= GTF_CHK_INDEX_INBND;
+        // We don't have the right kind of assertions to optimize bounds checks in local assertion prop.
         return nullptr;
     }
-#endif // FEATURE_ENABLE_NO_RANGE_CHECKS
 
     GenTreeBoundsChk* arrBndsChk    = tree->AsBoundsChk();
     GenTree*          arrBndsChkIdx = arrBndsChk->GetIndex();
     GenTree*          arrBndsChkLen = arrBndsChk->GetArrayLength();
-    ValueNum          vnCurIdx      = vnStore->VNConservativeNormalValue(arrBndsChkIdx->gtVNPair);
-    ValueNum          vnCurLen      = vnStore->VNConservativeNormalValue(arrBndsChkLen->gtVNPair);
+    ValueNum          vnCurIdx      = optConservativeNormalVN(arrBndsChkIdx);
+    ValueNum          vnCurLen      = optConservativeNormalVN(arrBndsChkLen);
 
     auto dropBoundsCheck = [&](INDEBUG(const char* reason)) -> GenTree* {
-        JITDUMP("\nVN based redundant (%s) bounds check assertion prop in " FMT_BB ":\n", reason, compCurBB->bbNum);
+        JITDUMP("\nRemoving redundant (%s) bounds check in " FMT_BB ":\n", reason, compCurBB->bbNum);
         DISPTREE(tree);
-        if (arrBndsChk != stmt->GetRootNode())
-        {
-            // Defer the removal.
-            arrBndsChk->gtFlags |= GTF_CHK_INDEX_INBND;
-            return nullptr;
-        }
 
-        GenTree* newTree = optRemoveStandaloneRangeCheck(arrBndsChk, stmt);
-        return optAssertionProp_Update(newTree, arrBndsChk, stmt);
+        // Extract the side effects of idx and len. We deliberately ignore the potential null-check
+        // exception of the length (e.g. GT_ARR_LENGTH): having proven the bounds check redundant, we
+        // have also proven the length load is non-faulting. This mirrors the hack in optRemoveRangeCheck.
+        GenTree* sideEffList = nullptr;
+        gtExtractSideEffList(arrBndsChkLen, &sideEffList, GTF_ASG);
+        gtExtractSideEffList(arrBndsChkIdx, &sideEffList);
+
+        GenTree* nothing = (sideEffList != nullptr) ? sideEffList : gtNewNothingNode();
+        return optAssertionProp_Update(nothing, arrBndsChk, stmt);
     };
 
     BitVecOps::Iter iter(apTraits, assertions);
     unsigned        index = 0;
     while (iter.NextElem(&index))
     {
-        AssertionIndex assertionIndex = GetAssertionIndex(index);
-        if (assertionIndex > optAssertionCount)
-        {
-            break;
-        }
         // If it is not a nothrow assertion, skip.
-        const AssertionDsc& curAssertion = optGetAssertion(assertionIndex);
+        const AssertionDsc& curAssertion = optGetAssertion(GetAssertionIndex(index));
         if (!curAssertion.IsBoundsCheckNoThrow())
         {
             continue;
@@ -5695,6 +5659,7 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions,
             return dropBoundsCheck(INDEBUG("upper bound of index is less than lower bound of length"));
         }
     }
+
     return nullptr;
 }
 
@@ -5843,9 +5808,6 @@ GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree,
         case GT_BOUNDS_CHECK:
             return optAssertionProp_BndsChk(assertions, tree, stmt, block);
 
-        case GT_COMMA:
-            return optAssertionProp_Comma(assertions, tree, stmt);
-
         case GT_CAST:
             return optAssertionProp_Cast(assertions, tree->AsCast(), stmt, block);
 

src/coreclr/jit/compiler.h

@@ -7523,7 +7523,6 @@ class Compiler
 public:
     PhaseStatus rangeCheckPhase();
     GenTree* optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, Statement* stmt);
-    GenTree* optRemoveStandaloneRangeCheck(GenTreeBoundsChk* check, Statement* stmt);
     void optRemoveCommaBasedRangeCheck(GenTree* comma, Statement* stmt);
 
 protected:
@@ -9233,7 +9232,6 @@ class Compiler
     GenTree* optAssertionProp_Cast(ASSERT_VALARG_TP assertions, GenTreeCast* cast, Statement* stmt, BasicBlock* block);
     GenTree* optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call, Statement* stmt);
     GenTree* optAssertionProp_RelOp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt, BasicBlock* block);
-    GenTree* optAssertionProp_Comma(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt);
     GenTree* optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt, BasicBlock* block);
     GenTree* optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions,
                                           GenTree*         tree,

src/coreclr/jit/gentree.h

@@ -528,8 +528,6 @@ enum GenTreeFlags : unsigned
 
     GTF_DIV_MOD_NO_OVERFLOW     = 0x40000000, // GT_DIV, GT_MOD -- Div or mod definitely does not overflow.
 
-    GTF_CHK_INDEX_INBND         = 0x80000000, // GT_BOUNDS_CHECK -- have proven this check is always in-bounds
-
     GTF_ARRLEN_NONFAULTING      = 0x20000000, // GT_ARR_LENGTH  -- An array length operation that cannot fault. Same as GT_IND_NONFAULTING.
 
     GTF_MDARRLEN_NONFAULTING    = 0x20000000, // GT_MDARR_LENGTH -- An MD array length operation that cannot fault. Same as GT_IND_NONFAULTING.

src/coreclr/jit/jitconfigvalues.h

@@ -576,10 +576,6 @@ RELEASE_CONFIG_INTEGER(JitInlineSIMDMultiplier, "JitInlineSIMDMultiplier", 3)
 // Ex lclMAX_TRACKED constant.
 RELEASE_CONFIG_INTEGER(JitMaxLocalsToTrack, "JitMaxLocalsToTrack", 0x400)
 
-#if defined(FEATURE_ENABLE_NO_RANGE_CHECKS)
-RELEASE_CONFIG_INTEGER(JitNoRngChks, "JitNoRngChks", 0) // If 1, don't generate range checks
-#endif
-
 OPT_CONFIG_INTEGER(JitDoAssertionProp, "JitDoAssertionProp", 1) // Perform assertion propagation optimization
 OPT_CONFIG_INTEGER(JitDoCopyProp, "JitDoCopyProp", 1) // Perform copy propagation on variables that appear redundant
 OPT_CONFIG_INTEGER(JitDoOptimizeIVs, "JitDoOptimizeIVs", 1)     // Perform optimization of induction variables

src/coreclr/jit/optimizer.cpp

@@ -5674,26 +5674,6 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma,
     return check;
 }
 
-//------------------------------------------------------------------------------
-// optRemoveStandaloneRangeCheck : A thin wrapper over optRemoveRangeCheck that removes standalone checks.
-//
-// Arguments:
-//    check - The standalone top-level CHECK node.
-//    stmt  - The statement "check" is a root node of.
-//
-// Return Value:
-//    If "check" has no side effects, it is retuned, bashed to a no-op.
-//    If it has side effects, the tree that executes them is returned.
-//
-GenTree* Compiler::optRemoveStandaloneRangeCheck(GenTreeBoundsChk* check, Statement* stmt)
-{
-    assert(check != nullptr);
-    assert(stmt != nullptr);
-    assert(check == stmt->GetRootNode());
-
-    return optRemoveRangeCheck(check, nullptr, stmt);
-}
-
 //------------------------------------------------------------------------------
 // optRemoveCommaBasedRangeCheck : A thin wrapper over optRemoveRangeCheck that removes COMMA-based checks.
 //