Unify O2K_CHECKED_BOUND_ADD_CNS and O2K_VN (#128165)

I decided that it makes no sense to have these separate. So now the
semantics of `O2K_VN_ADD_CNS`:

1) Decomposed "X+CNS" into `op2.GetVN()` being X and CNS being
`op2.GetCNS()`. Exactly like `O2K_CHECKED_BOUND_ADD_CNS` used to be. The
decomposition allows us to avoid calling GetVNFunc in the hot loop in
MergeEdgeAssertions since we usually are more interested in that X
rather than the whole ADD's VN.
2) `O2K_VN_ADD_CNS` alone doesn't imply that anything is non-negative
and no callers assume that (we used to have a bug where IsVNCheckedBound
was assumed to be unconditionally never-negative). There is
`GetOp2().IsVNNeverNegative()` which only exists because bound check
node actually produce two assertions we can't spawn today because trees
may only have 1 assertion. I'm thinking of solving this by introducing
an aggregate (or just allow trees to have 2 assertions), or just add a
fake COMMA
3) I intentionally tried to make this almost 0 diffs, but there are just
a few improvements I couldn't shut (they're not related to the
non-negativitiy).

Author: EgorBo

src/coreclr/jit/assertionprop.cpp

@@ -899,13 +899,8 @@ void Compiler::optPrintAssertion(const AssertionDsc& curAssertion, AssertionInde
             IntegralRange::Print(curAssertion.GetOp2().GetIntegralRange());
             break;
 
-        case O2K_CHECKED_BOUND_ADD_CNS:
-            printf("(Checked_Bnd_BinOp " FMT_VN " + %d)", curAssertion.GetOp2().GetCheckedBound(),
-                   curAssertion.GetOp2().GetCheckedBoundConstant());
-            break;
-
-        case O2K_VN:
-            printf("VN " FMT_VN "", curAssertion.GetOp2().GetVN());
+        case O2K_VN_ADD_CNS:
+            printf("(VN_ADD_CNS " FMT_VN " + %d)", curAssertion.GetOp2().GetVN(), curAssertion.GetOp2().GetCns());
             break;
 
         default:
@@ -1413,9 +1408,9 @@ AssertionIndex Compiler::optAddAssertion(const AssertionDsc& newAssertion)
         bool mayHaveDuplicates =
             optAssertionHasAssertionsForVN(newAssertion.GetOp1().GetVN(), /* addIfNotFound */ canAddNewAssertions);
         // We need to register op2.vn too, even if we know for sure there are no duplicates
-        if (newAssertion.GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS))
+        if (newAssertion.GetOp2().KindIs(O2K_VN_ADD_CNS))
         {
-            mayHaveDuplicates |= optAssertionHasAssertionsForVN(newAssertion.GetOp2().GetCheckedBound(),
+            mayHaveDuplicates |= optAssertionHasAssertionsForVN(newAssertion.GetOp2().GetVN(),
                                                                 /* addIfNotFound */ canAddNewAssertions);
 
             // Additionally, check for the pattern of "VN + const == checkedBndVN" and register "VN" as well.
@@ -1425,13 +1420,6 @@ AssertionIndex Compiler::optAddAssertion(const AssertionDsc& newAssertion)
                 mayHaveDuplicates |= optAssertionHasAssertionsForVN(addOpVN, /* addIfNotFound */ canAddNewAssertions);
             }
         }
-        else if (newAssertion.GetOp2().KindIs(O2K_VN))
-        {
-            // For VN <relop> VN assertions, register op2's VN too so consumers can find
-            // the assertion when iterating from the op2 side.
-            mayHaveDuplicates |= optAssertionHasAssertionsForVN(newAssertion.GetOp2().GetVN(),
-                                                                /* addIfNotFound */ canAddNewAssertions);
-        }
 
         if (mayHaveDuplicates)
         {
@@ -1561,7 +1549,7 @@ void Compiler::optDebugCheckAssertion(const AssertionDsc& assertion) const
             assert(optLocalAssertionProp);
             break;
 
-        case O2K_VN:
+        case O2K_VN_ADD_CNS:
             assert(!optLocalAssertionProp);
             assert(assertion.GetOp1().KindIs(O1K_VN));
             assert(assertion.IsRelop());
@@ -1650,8 +1638,7 @@ void Compiler::optCreateComplementaryAssertion(AssertionIndex assertionIndex)
         AssertionDsc reversed = candidateAssertion.Reverse();
         optMapComplementary(optAddAssertion(reversed), assertionIndex);
     }
-    else if (candidateAssertion.KindIs(OAK_LT_UN, OAK_LE_UN) &&
-             candidateAssertion.GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS))
+    else if (candidateAssertion.KindIs(OAK_LT_UN, OAK_LE_UN) && candidateAssertion.GetOp2().KindIs(O2K_VN_ADD_CNS))
     {
         // Assertions such as "X > checkedBndVN" aren't very useful.
         return;
@@ -4360,11 +4347,13 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions,
             // Look for a relop-like assertion that matches the current relop exactly.
             // Example: currentTree is "X >= Y" and we have an assertion "X >= Y" (or its inverse "X < Y").
             //
+            // For O2K_VN_ADD_CNS the assertion stores op2 as "vn + cns".
+            // - When cns == 0, the stored VN equals the original op2 VN, so direct match works.
+            // - When cns != 0, we'd need to assemble ADD(vn, cns) at the VN level; skip for now.
+            //
             if (curAssertion.IsRelop() && (curAssertion.GetOp1().GetVN() == op1VN) &&
-                // O2K_CHECKED_BOUND_ADD_CNS means op2 is decomposed into op2.vn being checked bound
-                // and op2.cns being the constant offset. We assemble the original relop VN if we want.
-                // For now, just skip such assertions here.
-                !curAssertion.GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS) && (curAssertion.GetOp2().GetVN() == op2VN))
+                (!curAssertion.GetOp2().KindIs(O2K_VN_ADD_CNS) || (curAssertion.GetOp2().GetCns() == 0)) &&
+                (curAssertion.GetOp2().GetVN() == op2VN))
             {
                 bool       isUnsigned;
                 genTreeOps assertionOper = AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned);
@@ -5449,12 +5438,11 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
             continue;
         }
 
-        assert(curAssertion.GetOp2().GetCheckedBoundConstant() == 0);
-        assert(curAssertion.GetOp2().IsCheckedBoundNeverNegative());
+        assert(curAssertion.GetOp2().GetCns() == 0);
+        assert(curAssertion.GetOp2().IsVNNeverNegative());
 
         // Do we have a previous range check involving the same 'vnLen' upper bound?
-        if (curAssertion.GetOp2().GetCheckedBound() ==
-            vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair))
+        if (curAssertion.GetOp2().GetVN() == optConservativeNormalVN(arrBndsChk->GetArrayLength()))
         {
             // Do we have the exact same lower bound 'vnIdx'?
             //       a[i] followed by a[i]

src/coreclr/jit/compiler.h

@@ -8192,14 +8192,10 @@ class Compiler
         O2K_CONST_INT,
         O2K_CONST_DOUBLE,
 
-        O2K_CHECKED_BOUND_ADD_CNS, // "checkedBndVN + cns" where op2.vn holds the "checkedBndVN"
-                                   // and op2.iconVal holds the "cns".
-                                   // "Checked bound" alone doesn't mean anything,
-                                   // nor it implies that it's never negative.
+        O2K_VN_ADD_CNS, // op2 represents an arbitrary VN (op2.GetVN()) plus a constant (op2.GetCns()).
         O2K_ZEROOBJ,
         O2K_SUBRANGE,
         O2K_CONST_VEC,
-        O2K_VN, // op2 is an arbitrary value number (used for VN <relop> VN assertions in global prop).
     };
 
     struct AssertionDsc
@@ -8258,7 +8254,7 @@ class Compiler
         private:
             INDEBUG(const Compiler* m_compiler);
             optOp2Kind m_kind;
-            bool       m_checkedBoundIsNeverNegative; // only meaningful for O2K_CHECKED_BOUND_ADD_CNS kind
+            bool       m_isVNNeverNegative; // only meaningful for O2K_VN_ADD_CNS kind
             union
             {
                 uint16_t m_encodedIconFlags; // encoded icon gtFlags; only meaningful for O2K_CONST_INT.
@@ -8345,35 +8341,26 @@ class Compiler
             ValueNum GetVN() const
             {
                 assert(!m_compiler->optLocalAssertionProp);
-                assert(KindIs(O2K_CONST_INT, O2K_CONST_DOUBLE, O2K_ZEROOBJ, O2K_CONST_VEC, O2K_VN));
+                assert(KindIs(O2K_CONST_INT, O2K_CONST_DOUBLE, O2K_ZEROOBJ, O2K_CONST_VEC, O2K_VN_ADD_CNS));
                 assert(m_vn != ValueNumStore::NoVN);
                 return m_vn;
             }
 
-            // For "checkedBndVN + cns" form, return the "cns" part.
-            int GetCheckedBoundConstant() const
+            // For "vn + cns" form, return the "cns" part.
+            int GetCns() const
             {
                 assert(!m_compiler->optLocalAssertionProp);
-                assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS));
+                assert(KindIs(O2K_VN_ADD_CNS));
                 assert(FitsIn<int>(m_icon.m_iconVal));
                 return (int)m_icon.m_iconVal;
             }
 
-            // For "checkedBndVN + cns" form, return the "checkedBndVN" part.
-            // We intentionally don't allow to use it via GetVN() to avoid confusion.
-            ValueNum GetCheckedBound() const
+            // For "vn + cns" form, true iff the JIT has proven the "vn" part to be non-negative.
+            bool IsVNNeverNegative() const
             {
                 assert(!m_compiler->optLocalAssertionProp);
-                assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS));
-                assert(m_vn != ValueNumStore::NoVN);
-                return m_vn;
-            }
-
-            bool IsCheckedBoundNeverNegative() const
-            {
-                assert(!m_compiler->optLocalAssertionProp);
-                assert(KindIs(O2K_CHECKED_BOUND_ADD_CNS));
-                return m_checkedBoundIsNeverNegative;
+                assert(KindIs(O2K_VN_ADD_CNS));
+                return m_isVNNeverNegative;
             }
 
             optOp2Kind GetKind() const
@@ -8500,10 +8487,10 @@ class Compiler
 
         bool IsBoundsCheckNoThrow() const
         {
-            // O1K_VN (idx) u< O2K_VN (len) where len is never negative.
+            // O1K_VN (idx) u< O2K_VN_ADD_CNS (len) where len is never negative.
             // Effectively, it's "idx >= 0 && idx < len"
-            return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_CHECKED_BOUND_ADD_CNS) &&
-                   (GetOp2().GetCheckedBoundConstant() == 0) && GetOp2().IsCheckedBoundNeverNegative();
+            return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_VN_ADD_CNS) &&
+                   (GetOp2().GetCns() == 0) && GetOp2().IsVNNeverNegative();
         }
 
         // Convert VNFunc to optAssertionKind
@@ -8685,20 +8672,16 @@ class Compiler
                 case O2K_ZEROOBJ:
                     return true;
 
-                case O2K_CHECKED_BOUND_ADD_CNS:
-                    return GetOp2().GetCheckedBound() == that.GetOp2().GetCheckedBound() &&
-                           GetOp2().GetCheckedBoundConstant() == that.GetOp2().GetCheckedBoundConstant() &&
-                           GetOp2().IsCheckedBoundNeverNegative() == that.GetOp2().IsCheckedBoundNeverNegative();
+                case O2K_VN_ADD_CNS:
+                    return GetOp2().GetVN() == that.GetOp2().GetVN() && GetOp2().GetCns() == that.GetOp2().GetCns() &&
+                           GetOp2().IsVNNeverNegative() == that.GetOp2().IsVNNeverNegative();
 
                 case O2K_LCLVAR_COPY:
                     return GetOp2().GetLclNum() == that.GetOp2().GetLclNum();
 
                 case O2K_SUBRANGE:
                     return GetOp2().GetIntegralRange().Equals(that.GetOp2().GetIntegralRange());
 
-                case O2K_VN:
-                    return GetOp2().GetVN() == that.GetOp2().GetVN();
-
                 default:
                     assert(!"Unexpected value for GetOp2().m_kind in AssertionDsc.");
                     break;
@@ -8914,12 +8897,13 @@ class Compiler
             dsc.m_assertionKind = OAK_LT_UN;
             dsc.m_op1.m_kind    = O1K_VN;
             dsc.m_op1.m_vn      = idxVN;
-            dsc.m_op2.m_kind    = O2K_CHECKED_BOUND_ADD_CNS;
+            dsc.m_op2.m_kind    = O2K_VN_ADD_CNS;
             dsc.m_op2.m_vn      = lenVN;
 
-            // Normally, "Checked bound" doesn't mean it's never negative, but in this particular case we know it is.
-            dsc.m_op2.m_checkedBoundIsNeverNegative = true;
-            dsc.m_op2.m_icon.m_iconVal              = 0;
+            // Normally, the VN of an O2K_VN_ADD_CNS assertion isn't proven non-negative, but
+            // in this particular case we know it is (it's a length).
+            dsc.m_op2.m_isVNNeverNegative = true;
+            dsc.m_op2.m_icon.m_iconVal    = 0;
             return dsc;
         }
 
@@ -8934,7 +8918,7 @@ class Compiler
             dsc.m_assertionKind        = FromVNFunc(relop);
             dsc.m_op1.m_kind           = O1K_VN;
             dsc.m_op1.m_vn             = op1VN;
-            dsc.m_op2.m_kind           = O2K_CHECKED_BOUND_ADD_CNS;
+            dsc.m_op2.m_kind           = O2K_VN_ADD_CNS;
             dsc.m_op2.m_vn             = checkedBndVN;
             dsc.m_op2.m_icon.m_iconVal = cns;
             return dsc;
@@ -8968,12 +8952,14 @@ class Compiler
             assert(op2VN != ValueNumStore::NoVN);
             assert(op1VN != op2VN);
 
-            AssertionDsc dsc    = CreateEmptyAssertion(comp);
-            dsc.m_assertionKind = FromVNFunc(relop);
-            dsc.m_op1.m_kind    = O1K_VN;
-            dsc.m_op1.m_vn      = op1VN;
-            dsc.m_op2.m_kind    = O2K_VN;
-            dsc.m_op2.m_vn      = op2VN;
+            AssertionDsc dsc              = CreateEmptyAssertion(comp);
+            dsc.m_assertionKind           = FromVNFunc(relop);
+            dsc.m_op1.m_kind              = O1K_VN;
+            dsc.m_op1.m_vn                = op1VN;
+            dsc.m_op2.m_kind              = O2K_VN_ADD_CNS;
+            dsc.m_op2.m_vn                = op2VN;
+            dsc.m_op2.m_icon.m_iconVal    = 0; // TODO-CQ: consider decomposing VN to VN* + CNS if beneficial.
+            dsc.m_op2.m_isVNNeverNegative = false;
             return dsc;
         }
     };

src/coreclr/jit/rangecheck.cpp

@@ -1126,11 +1126,10 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
         // We can deduce normalLclVN's upper bound to be preferredBoundVN - CNS1
         // Example: "(uint)(i + 2) < (uint)span.Length"
         if (canUseCheckedBounds && curAssertion.KindIs(Compiler::OAK_LT_UN) &&
-            curAssertion.GetOp2().KindIs(Compiler::O2K_CHECKED_BOUND_ADD_CNS) &&
+            curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) &&
             // Normally, checked bound doesn't directly imply non-negativity, so we check
             // that explicitly here:
-            curAssertion.GetOp2().IsCheckedBoundNeverNegative() &&
-            (curAssertion.GetOp2().GetCheckedBound() == preferredBoundVN) &&
+            curAssertion.GetOp2().IsVNNeverNegative() && (curAssertion.GetOp2().GetVN() == preferredBoundVN) &&
             (curAssertion.GetOp1().GetVN() != normalLclVN))
         {
             ValueNum addOpVN;
@@ -1164,21 +1163,20 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
             }
         }
         // If we're not allowed to emit keBinOpArray, we still can deduce something useful from
-        // O2K_CHECKED_BOUND_ADD_CNS for never negative checked bounds
+        // O2K_VN_ADD_CNS for never negative checked bounds
         //
         //  Example: "(uint)normalLclVN < span.Length"   means normalLclVN's range is [0..INT32_MAX-1]
         //  Example: "(uint)normalLclVN <= array.Length" means normalLclVN's range is [0..Array.MaxLength]
         //
         else if (!canUseCheckedBounds && curAssertion.IsRelop() && (curAssertion.GetOp1().GetVN() == normalLclVN) &&
-                 (curAssertion.GetOp2().KindIs(Compiler::O2K_CHECKED_BOUND_ADD_CNS)) &&
-                 (curAssertion.GetOp2().IsCheckedBoundNeverNegative()) &&
-                 (curAssertion.GetOp2().GetCheckedBoundConstant() == 0))
+                 (curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS)) &&
+                 (curAssertion.GetOp2().IsVNNeverNegative()) && (curAssertion.GetOp2().GetCns() == 0))
         {
-            // We can assume that the checked bound is within [0..INT32_MAX] thanks to IsCheckedBoundNeverNegative,
+            // We can assume that the checked bound is within [0..INT32_MAX] thanks to IsVNNeverNegative,
             // but let's see if we can do better: we could call GetRangeFromAssertions on the checked bound's VN,
             // but that may lead to infinite recursion. Also, the checked bound is usually either arr.Length or
             // span.Length anyway.
-            ValueNum checkedBoundVN = curAssertion.GetOp2().GetCheckedBound();
+            ValueNum checkedBoundVN = curAssertion.GetOp2().GetVN();
 
             int maxValue = INT32_MAX;
             if (comp->vnStore->IsVNArrLen(checkedBoundVN))
@@ -1197,12 +1195,18 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
             cmpOper = Compiler::AssertionDsc::ToCompareOper(curAssertion.GetKind(), &isUnsigned);
             limit   = Limit(Limit::keConstant, maxValue);
         }
-        // Current assertion is of the form "i <relop> (checkedBndVN + cns)"
+        // Current assertion is of the form "i <relop> (vn + cns)" where vn is a real
+        // (length-like) checked bound. The arbitrary-VN sub-form of O2K_VN_ADD_CNS (created by
+        // CreateRelopVN, where op2.vn is not a checked bound) is intentionally excluded here so
+        // that it never flows into a keBinOpArray Limit, whose Range::IsValid / Range::Widen
+        // rules implicitly assume the VN refers to a length-like (checked-bound-shaped) quantity.
+        // Such cases fall through to the general "X <relop> Y" branch below.
         else if (curAssertion.KindIs(Compiler::OAK_GE, Compiler::OAK_GT, Compiler::OAK_LE, Compiler::OAK_LT) &&
-                 curAssertion.GetOp2().KindIs(Compiler::O2K_CHECKED_BOUND_ADD_CNS))
+                 curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) &&
+                 comp->vnStore->IsVNCheckedBound(curAssertion.GetOp2().GetVN()))
         {
-            const ValueNum boundVN  = curAssertion.GetOp2().GetCheckedBound();
-            const int      boundCns = curAssertion.GetOp2().GetCheckedBoundConstant();
+            const ValueNum boundVN  = curAssertion.GetOp2().GetVN();
+            const int      boundCns = curAssertion.GetOp2().GetCns();
 
             // Check whether normalLclVN VN-equals the op2 expression "(boundVN + boundCns)":
             //  - trivially, when boundCns is 0 and normalLclVN == boundVN, or
@@ -1234,7 +1238,7 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
                 // Check if it's "Const <relop> (checkedBndVN + cns)" or in other words "Const <relop> normalLclVN"
                 // If normalLclVN VN-equals (checkedBndVN + cns), we can deduce "normalLclVN <relop> Const"
                 //
-                // Example: We created "O1K_VN(vn1) - OAK_GT - O2K_CHECKED_BOUND_ADD_CNS(vn2, 0)"
+                // Example: We created "O1K_VN(vn1) - OAK_GT - O2K_VN_ADD_CNS(vn2, 0)"
                 // if vn1 is a constant (say, 100) and vn2 is our normalLclVN, we can deduce "normalLclVN < 100"
                 //
                 if (comp->vnStore->IsVNInt32Constant(curAssertion.GetOp1().GetVN()))
@@ -1321,10 +1325,10 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
         {
             // IsBoundsCheckNoThrow is "op1VN (Idx) LT_UN op2VN (Len)"
             ValueNum indexVN = curAssertion.GetOp1().GetVN();
-            ValueNum lenVN   = curAssertion.GetOp2().GetCheckedBound();
+            ValueNum lenVN   = curAssertion.GetOp2().GetVN();
 
-            assert(curAssertion.GetOp2().GetCheckedBoundConstant() == 0);
-            assert(curAssertion.GetOp2().IsCheckedBoundNeverNegative());
+            assert(curAssertion.GetOp2().GetCns() == 0);
+            assert(curAssertion.GetOp2().IsVNNeverNegative());
 
             if (normalLclVN == indexVN)
             {
@@ -1404,7 +1408,8 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler*                        comp
         //    if (normalLclVN > a) // a is an unknown VN with [11..99] range derived from assertions.
         //    {
         //
-        else if (curAssertion.IsRelop() && curAssertion.GetOp2().KindIs(Compiler::O2K_VN) &&
+        else if (curAssertion.IsRelop() && curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) &&
+                 (curAssertion.GetOp2().GetCns() == 0) &&
                  (curAssertion.GetOp1().GetVN() == normalLclVN || curAssertion.GetOp2().GetVN() == normalLclVN))
         {
             ValueNum op1VN = curAssertion.GetOp1().GetVN();