Fix issue where the unsigned value used for the the map select budget could underflow

resulting in an unlimited budget

Changed m_mapSelectBudget and budget to be a signed integer
Added an assert that ensures that the remaining budget is between [0..m_mapSelectBudget]
Change the test for running out of budge to be a <= zero test instead of an equal zero test.

Fixed the bug in the handling of phiArgs in VNForMapSelectWork
by adding a check if we exceeded our budget when processing the first phiArg.

Added a define for DEFAULT_MAP_SELECT_BUDGET

Author: briansull

src/jit/jitconfigvalues.h

@@ -282,11 +282,13 @@ CONFIG_METHODSET(JitOptRepeat, W("JitOptRepeat"))            // Runs optimizer m
 CONFIG_INTEGER(JitOptRepeatCount, W("JitOptRepeatCount"), 2) // Number of times to repeat opts when repeating
 #endif                                                       // defined(OPT_CONFIG)
 
-CONFIG_INTEGER(JitRegisterFP, W("JitRegisterFP"), 3)           // Control FP enregistration
-CONFIG_INTEGER(JitTelemetry, W("JitTelemetry"), 1)             // If non-zero, gather JIT telemetry data
-CONFIG_INTEGER(JitVNMapSelBudget, W("JitVNMapSelBudget"), 100) // Max # of MapSelect's considered for a particular
-                                                               // top-level invocation.
-CONFIG_INTEGER(TailCallLoopOpt, W("TailCallLoopOpt"), 1)       // Convert recursive tail calls to loops
+CONFIG_INTEGER(JitRegisterFP, W("JitRegisterFP"), 3) // Control FP enregistration
+CONFIG_INTEGER(JitTelemetry, W("JitTelemetry"), 1)   // If non-zero, gather JIT telemetry data
+
+// Max # of MapSelect's considered for a particular top-level invocation.
+CONFIG_INTEGER(JitVNMapSelBudget, W("JitVNMapSelBudget"), DEFAULT_MAP_SELECT_BUDGET)
+
+CONFIG_INTEGER(TailCallLoopOpt, W("TailCallLoopOpt"), 1) // Convert recursive tail calls to loops
 CONFIG_METHODSET(AltJit, W("AltJit")) // Enables AltJit and selectively limits it to the specified methods.
 CONFIG_METHODSET(AltJitNgen,
                  W("AltJitNgen")) // Enables AltJit for NGEN and selectively limits it to the specified methods.

src/jit/valuenum.cpp

@@ -95,7 +95,13 @@ ValueNumStore::ValueNumStore(Compiler* comp, CompAllocator* alloc)
     ChunkNum cn = m_chunks.Push(specialConstChunk);
     assert(cn == 0);
 
-    m_mapSelectBudget = JitConfig.JitVNMapSelBudget();
+    m_mapSelectBudget = (int)JitConfig.JitVNMapSelBudget(); // We cast the unsigned DWORD to a signed int.
+
+    // This value must be non-negative and non-zero, reset the value to DEFAULT_MAP_SELECT_BUDGET if it isn't.
+    if (m_mapSelectBudget <= 0)
+    {
+        m_mapSelectBudget = DEFAULT_MAP_SELECT_BUDGET;
+    }
 }
 
 // static.
@@ -1326,9 +1332,13 @@ ValueNum ValueNumStore::VNForMapStore(var_types typ, ValueNum arg0VN, ValueNum a
 
 ValueNum ValueNumStore::VNForMapSelect(ValueNumKind vnk, var_types typ, ValueNum arg0VN, ValueNum arg1VN)
 {
-    unsigned budget          = m_mapSelectBudget;
+    int      budget          = m_mapSelectBudget;
     bool     usedRecursiveVN = false;
     ValueNum result          = VNForMapSelectWork(vnk, typ, arg0VN, arg1VN, &budget, &usedRecursiveVN);
+
+    // The remaining budget should always be between [0..m_mapSelectBudget]
+    assert((budget >= 0) && (budget <= m_mapSelectBudget));
+
 #ifdef DEBUG
     if (m_pComp->verbose)
     {
@@ -1362,7 +1372,7 @@ ValueNum ValueNumStore::VNForMapSelect(ValueNumKind vnk, var_types typ, ValueNum
 //    (liberal/conservative) to read from the SSA def referenced in the phi argument.
 
 ValueNum ValueNumStore::VNForMapSelectWork(
-    ValueNumKind vnk, var_types typ, ValueNum arg0VN, ValueNum arg1VN, unsigned* pBudget, bool* pUsedRecursiveVN)
+    ValueNumKind vnk, var_types typ, ValueNum arg0VN, ValueNum arg1VN, int* pBudget, bool* pUsedRecursiveVN)
 {
 TailCall:
     // This label allows us to directly implement a tail call by setting up the arguments, and doing a goto to here.
@@ -1393,7 +1403,7 @@ ValueNum ValueNumStore::VNForMapSelectWork(
     {
 
         // Give up if we've run out of budget.
-        if (--(*pBudget) == 0)
+        if (--(*pBudget) <= 0)
         {
             // We have to use 'nullptr' for the basic block here, because subsequent expressions
             // in different blocks may find this result in the VNFunc2Map -- other expressions in
@@ -1491,6 +1501,16 @@ ValueNum ValueNumStore::VNForMapSelectWork(
                         ValueNum argRest = phiFuncApp.m_args[1];
                         ValueNum sameSelResult =
                             VNForMapSelectWork(vnk, typ, phiArgVN, arg1VN, pBudget, pUsedRecursiveVN);
+
+                        // It is possible that we just now exceeded our budget, if so we need to force an early exit
+                        // and stop calling VNForMapSelectWork
+                        if (*pBudget <= 0)
+                        {
+                            // We don't have any budget remaining to verify that all phiArgs are the same
+                            // so setup the default failure case now.
+                            allSame = false;
+                        }
+
                         while (allSame && argRest != ValueNumStore::NoVN)
                         {
                             ValueNum  cur = argRest;
@@ -6914,6 +6934,7 @@ void Compiler::fgValueNumberTree(GenTreePtr tree, bool evalAsgLhsInd)
                         ValueNumPair op1VNP;
                         ValueNumPair op1VNPx = ValueNumStore::VNPForEmptyExcSet();
                         vnStore->VNPUnpackExc(tree->gtOp.gtOp1->gtVNPair, &op1VNP, &op1VNPx);
+
                         tree->gtVNPair =
                             vnStore->VNPWithExc(vnStore->VNPairForFunc(tree->TypeGet(),
                                                                        GetVNFuncForOper(oper, (tree->gtFlags &

src/jit/valuenum.h

@@ -225,9 +225,12 @@ class ValueNumStore
     unsigned m_numMapSels;
 #endif
 
+// This is the constant value used for the default value of m_mapSelectBudget
+#define DEFAULT_MAP_SELECT_BUDGET 100 // used by JitVNMapSelBudget
+
     // This is the maximum number of MapSelect terms that can be "considered" as part of evaluation of a top-level
     // MapSelect application.
-    unsigned m_mapSelectBudget;
+    int m_mapSelectBudget;
 
 public:
     // Initializes any static variables of ValueNumStore.
@@ -406,7 +409,7 @@ class ValueNumStore
 
     // A method that does the work for VNForMapSelect and may call itself recursively.
     ValueNum VNForMapSelectWork(
-        ValueNumKind vnk, var_types typ, ValueNum op1VN, ValueNum op2VN, unsigned* pBudget, bool* pUsedRecursiveVN);
+        ValueNumKind vnk, var_types typ, ValueNum op1VN, ValueNum op2VN, int* pBudget, bool* pUsedRecursiveVN);
 
     // A specialized version of VNForFunc that is used for VNF_MapStore and provides some logging when verbose is set
     ValueNum VNForMapStore(var_types typ, ValueNum arg0VN, ValueNum arg1VN, ValueNum arg2VN);