From 442a55e396aa63fe5b3f8a12f934a290d4f3a023 Mon Sep 17 00:00:00 2001
From: Andy Ayers <andya@microsoft.com>
Date: Mon, 17 May 2021 20:12:56 -0700
Subject: [PATCH] JIT: don't allow negative edge weights

If the sovler wants to set the edge weight below zero, set it to
zero if within slop, or disallow if not.

Addresses assert seen in #52864.
---
 src/coreclr/jit/fgprofile.cpp | 91 +++++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 31 deletions(-)

diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp
index 3fb8254baf5728..142a2c7ac6656b 100644
--- a/src/coreclr/jit/fgprofile.cpp
+++ b/src/coreclr/jit/fgprofile.cpp
@@ -2577,7 +2577,26 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
                                        BasicBlock::weight_t slop,
                                        bool*                wbUsedSlop)
 {
+    // Negative weights are nonsensical.
+    //
+    // If we can't cover the deficit with slop, fail.
+    // If we can, set the new weight to zero.
+    //
+    bool usedSlop = false;
+
+    if (newWeight < BB_ZERO_WEIGHT)
+    {
+        if ((newWeight + slop) < BB_ZERO_WEIGHT)
+        {
+            return false;
+        }
+
+        newWeight = BB_ZERO_WEIGHT;
+        usedSlop  = true;
+    }
+
     bool result = false;
+
     if ((newWeight <= flEdgeWeightMax) && (newWeight >= flEdgeWeightMin))
     {
         flEdgeWeightMin = newWeight;
@@ -2592,7 +2611,8 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
             // is less than newWeight, so we just allow for the slop
             if (newWeight <= (flEdgeWeightMax + slop))
             {
-                result = true;
+                result   = true;
+                usedSlop = true;
 
                 if (flEdgeWeightMax != BB_ZERO_WEIGHT)
                 {
@@ -2600,11 +2620,6 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
                     flEdgeWeightMin = flEdgeWeightMax;
                     flEdgeWeightMax = newWeight;
                 }
-
-                if (wbUsedSlop != nullptr)
-                {
-                    *wbUsedSlop = true;
-                }
             }
         }
         else if (flEdgeWeightMin > newWeight)
@@ -2613,33 +2628,34 @@ bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight,
             // is more than newWeight, so we just allow for the slop
             if ((newWeight + slop) >= flEdgeWeightMin)
             {
-                result = true;
+                result   = true;
+                usedSlop = true;
 
                 if (flEdgeWeightMax != BB_ZERO_WEIGHT)
                 {
                     // We will lower flEdgeWeightMin towards newWeight
-                    flEdgeWeightMin = newWeight;
-                }
-
-                if (wbUsedSlop != nullptr)
-                {
-                    *wbUsedSlop = true;
+                    // But not below zero.
+                    //
+                    flEdgeWeightMin = max(BB_ZERO_WEIGHT, newWeight);
                 }
             }
         }
 
         // If we are returning true then we should have adjusted the range so that
         // the newWeight is in new range [Min..Max] or fgEdgeWeightMax is zero.
-        // Also we should have set wbUsedSlop to true.
+        //
         if (result)
         {
             assert((flEdgeWeightMax == BB_ZERO_WEIGHT) ||
                    ((newWeight <= flEdgeWeightMax) && (newWeight >= flEdgeWeightMin)));
-
-            assert((wbUsedSlop == nullptr) || (*wbUsedSlop));
         }
     }
 
+    if (result && usedSlop && (wbUsedSlop != nullptr))
+    {
+        *wbUsedSlop = true;
+    }
+
 #if DEBUG
     if (result)
     {
@@ -2677,7 +2693,26 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
                                        BasicBlock::weight_t slop,
                                        bool*                wbUsedSlop)
 {
+    // Negative weights are nonsensical.
+    //
+    // If we can't cover the deficit with slop, fail.
+    // If we can, set the new weight to zero.
+    //
+    bool usedSlop = false;
+
+    if (newWeight < BB_ZERO_WEIGHT)
+    {
+        if ((newWeight + slop) < BB_ZERO_WEIGHT)
+        {
+            return false;
+        }
+
+        newWeight = BB_ZERO_WEIGHT;
+        usedSlop  = true;
+    }
+
     bool result = false;
+
     if ((newWeight >= flEdgeWeightMin) && (newWeight <= flEdgeWeightMax))
     {
         flEdgeWeightMax = newWeight;
@@ -2692,18 +2727,14 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
             // is less than newWeight, so we just allow for the slop
             if (newWeight <= (flEdgeWeightMax + slop))
             {
-                result = true;
+                result   = true;
+                usedSlop = true;
 
                 if (flEdgeWeightMax != BB_ZERO_WEIGHT)
                 {
                     // We will allow this to raise flEdgeWeightMax towards newWeight
                     flEdgeWeightMax = newWeight;
                 }
-
-                if (wbUsedSlop != nullptr)
-                {
-                    *wbUsedSlop = true;
-                }
             }
         }
         else if (flEdgeWeightMin > newWeight)
@@ -2712,7 +2743,8 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
             // is more than newWeight, so we just allow for the slop
             if ((newWeight + slop) >= flEdgeWeightMin)
             {
-                result = true;
+                result   = true;
+                usedSlop = true;
 
                 if (flEdgeWeightMax != BB_ZERO_WEIGHT)
                 {
@@ -2720,26 +2752,23 @@ bool flowList::setEdgeWeightMaxChecked(BasicBlock::weight_t newWeight,
                     flEdgeWeightMax = flEdgeWeightMin;
                     flEdgeWeightMin = newWeight;
                 }
-
-                if (wbUsedSlop != nullptr)
-                {
-                    *wbUsedSlop = true;
-                }
             }
         }
 
         // If we are returning true then we should have adjusted the range so that
         // the newWeight is in new range [Min..Max] or fgEdgeWeightMax is zero
-        // Also we should have set wbUsedSlop to true, unless it is NULL
         if (result)
         {
             assert((flEdgeWeightMax == BB_ZERO_WEIGHT) ||
                    ((newWeight <= flEdgeWeightMax) && (newWeight >= flEdgeWeightMin)));
-
-            assert((wbUsedSlop == nullptr) || (*wbUsedSlop));
         }
     }
 
+    if (result && usedSlop && (wbUsedSlop != nullptr))
+    {
+        *wbUsedSlop = true;
+    }
+
 #if DEBUG
     if (result)
     {