Merge pull request #6484 from CarolEidt/AssertionPropAsserts

Do not propagate bad assertions from unreachable blocks

3 files changed, 100 insertions, 42 deletions

diff --git a/src/jit/assertionprop.cpp b/src/jit/assertionprop.cpp
index 1ac1cd285f..781e46068b 100644
--- a/src/jit/assertionprop.cpp
+++ b/src/jit/assertionprop.cpp
@@ -754,12 +754,13 @@ Compiler::AssertionDsc *  Compiler::optGetAssertion(AssertionIndex assertIndex)
 {
     assert(NO_ASSERTION_INDEX == 0);
     noway_assert(assertIndex != NO_ASSERTION_INDEX);
+    noway_assert(assertIndex <= optAssertionCount);
+    AssertionDsc* assertion = &optAssertionTabPrivate[assertIndex - 1];
+#ifdef DEBUG
+    optDebugCheckAssertion(assertion);
+#endif
 
-    if (assertIndex > optMaxAssertionCount)
-    {
-        return nullptr;
-    }
-    return &optAssertionTabPrivate[assertIndex - 1];
+    return assertion;
 }
 
 /*****************************************************************************
@@ -1488,6 +1489,63 @@ Compiler::AssertionIndex Compiler::optAddAssertion(AssertionDsc* newAssertion)
 }
 
 #ifdef DEBUG
+void Compiler::optDebugCheckAssertion(AssertionDsc* assertion)
+{
+    assert(assertion->assertionKind < OAK_COUNT);
+    assert(assertion->op1.kind < O1K_COUNT);
+    assert(assertion->op2.kind < O2K_COUNT);
+    // It would be good to check that op1.vn and op2.vn are valid value numbers.
+
+    switch(assertion->op1.kind)
+    {
+    case O1K_LCLVAR:
+    case O1K_EXACT_TYPE:
+    case O1K_SUBTYPE:
+        assert(assertion->op1.lcl.lclNum < lvaCount);
+        assert(optLocalAssertionProp ||
+               ((assertion->op1.lcl.ssaNum - SsaConfig::UNINIT_SSA_NUM) < lvaTable[assertion->op1.lcl.lclNum].lvNumSsaNames));
+        break;
+    case O1K_ARR_BND:
+        // It would be good to check that bnd.vnIdx and bnd.vnLen are valid value numbers.
+        break;
+    case O1K_ARRLEN_OPER_BND:
+    case O1K_ARRLEN_LOOP_BND:
+    case O1K_CONSTANT_LOOP_BND:
+        assert(!optLocalAssertionProp);
+        break;
+    default:
+        break;
+    }
+    switch (assertion->op2.kind)
+    {
+    case O2K_IND_CNS_INT:
+    case O2K_CONST_INT:
+    {
+        // The only flags that can be set are those in the GTF_ICON_HDL_MASK, or bit 0, which is
+        // used to indicate a long constant.
+        assert((assertion->op2.u1.iconFlags & ~(GTF_ICON_HDL_MASK|1)) == 0);
+        switch (assertion->op1.kind)
+        {
+        case O1K_EXACT_TYPE:
+        case O1K_SUBTYPE:
+            assert(assertion->op2.u1.iconFlags != 0);
+            break;
+        case O1K_LCLVAR:
+        case O1K_ARR_BND:
+            assert(lvaTable[assertion->op1.lcl.lclNum].lvType != TYP_REF || assertion->op2.u1.iconVal == 0);
+            break;
+        default:
+            break;
+        }
+    }
+    break;
+
+    default:
+        // for all other 'assertion->op2.kind' values we don't check anything
+        break;
+    }
+}
+
 /*****************************************************************************
  *
  *  Verify that assertion prop related assumptions are valid. If "index"
@@ -1502,33 +1560,7 @@ void Compiler::optDebugCheckAssertions(AssertionIndex index)
     for (AssertionIndex ind = start; ind <= end; ++ind)
     {
         AssertionDsc* assertion = optGetAssertion(ind);
-        switch (assertion->op2.kind)
-        {
-        case O2K_IND_CNS_INT:
-        case O2K_CONST_INT:
-           {
-               switch (assertion->op1.kind)
-               {
-               case O1K_EXACT_TYPE:
-               case O1K_SUBTYPE:
-                   assert(assertion->op2.u1.iconFlags != 0);
-                   break;
-               case O1K_ARRLEN_OPER_BND:
-               case O1K_ARRLEN_LOOP_BND:
-               case O1K_CONSTANT_LOOP_BND:
-                   assert(!optLocalAssertionProp);
-                   break;
-               default:
-                   assert(lvaTable[assertion->op1.lcl.lclNum].lvType != TYP_REF || assertion->op2.u1.iconVal == 0);
-                   break;
-               }
-           }
-           break;
-
-        default:
-            // for all other 'assertion->op2.kind' values we don't check anything
-           break;
-        }
+        optDebugCheckAssertion(assertion);
     }
 }
 #endif
@@ -4282,6 +4314,18 @@ ASSERT_TP* Compiler::optInitAssertionDataflowFlags()
 {
     ASSERT_TP* jumpDestOut = fgAllocateTypeForEachBlk<ASSERT_TP>();
 
+    // The local assertion gen phase may have created unreachable blocks.
+    // They will never be visited in the dataflow propagation phase, so they need to
+    // be initialized correctly. This means that instead of setting their sets to
+    // apFull (i.e. all possible bits set), we need to set the bits only for valid
+    // assertions (note that at this point we are not creating any new assertions).
+    // Also note that assertion indices start from 1.
+    ASSERT_TP apValidFull = optNewEmptyAssertSet();
+    for (int i = 1; i <= optAssertionCount; i++) 
+    {
+        BitVecOps::AddElemD(apTraits, apValidFull, i-1);
+    }
+
     // Initially estimate the OUT sets to everything except killed expressions
     // Also set the IN sets to 1, so that we can perform the intersection.
     // Also, zero-out the flags for handler blocks, as we could be in the
@@ -4290,11 +4334,16 @@ ASSERT_TP* Compiler::optInitAssertionDataflowFlags()
     // edges.
     for (BasicBlock* block = fgFirstBB; block; block = block->bbNext)
     {
-        block->bbAssertionIn  = bbIsHandlerBeg(block) ? optNewEmptyAssertSet() : optNewFullAssertSet();
+        block->bbAssertionIn  = optNewEmptyAssertSet();
+        if (!bbIsHandlerBeg(block))
+        {
+             BitVecOps::Assign(apTraits, block->bbAssertionIn, apValidFull);
+        }
         block->bbAssertionGen = optNewEmptyAssertSet();
-        block->bbAssertionOut = optNewFullAssertSet();
+        block->bbAssertionOut = optNewEmptyAssertSet();
+        BitVecOps::Assign(apTraits, block->bbAssertionOut, apValidFull);
         jumpDestOut[block->bbNum] = optNewEmptyAssertSet();
-        BitVecOps::Assign(apTraits, jumpDestOut[block->bbNum], apFull);
+        BitVecOps::Assign(apTraits, jumpDestOut[block->bbNum], apValidFull);
     }
     // Compute the data flow values for all tracked expressions
     // IN and OUT never change for the initial basic block B1
@@ -4817,9 +4866,9 @@ void Compiler::optAssertionPropMain()
                                                            // and thus we must morph, set order, re-link
             for (GenTreePtr tree = stmt->gtStmt.gtStmtList; tree; tree = tree->gtNext)
             {
-                JITDUMP("Propagating %s assertions for BB%02d, stmt %08X, tree %08X, tree -> %d\n",
-                        BitVecOps::ToString(apTraits, assertions),
-                        block->bbNum, dspPtr(stmt), dspPtr(tree), tree->GetAssertion());
+                JITDUMP("Propagating %s assertions for BB%02d, stmt [%06d], tree [%06d], tree -> %d\n",
+                    BitVecOps::ToString(apTraits, assertions),
+                    block->bbNum, dspTreeID(stmt), dspTreeID(tree), tree->GetAssertion());
 
                 GenTreePtr newTree = optAssertionProp(assertions, tree, stmt);
                 if (newTree)
diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index 3eb2fdcb83..e3a4b519c7 100644
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -5560,7 +5560,8 @@ public:
                             OAK_EQUAL,
                             OAK_NOT_EQUAL, 
                             OAK_SUBRANGE,
-                            OAK_NO_THROW };
+                            OAK_NO_THROW,
+                            OAK_COUNT };
 
     enum optOp1Kind       { O1K_INVALID,
                             O1K_LCLVAR,
@@ -5569,7 +5570,8 @@ public:
                             O1K_ARRLEN_LOOP_BND,
                             O1K_CONSTANT_LOOP_BND,
                             O1K_EXACT_TYPE,
-                            O1K_SUBTYPE };
+                            O1K_SUBTYPE,
+                            O1K_COUNT };
 
     enum optOp2Kind       { O2K_INVALID,
                             O2K_LCLVAR_COPY,
@@ -5578,7 +5580,8 @@ public:
                             O2K_CONST_LONG,
                             O2K_CONST_DOUBLE,
                             O2K_ARR_LEN,
-                            O2K_SUBRANGE };
+                            O2K_SUBRANGE,
+                            O2K_COUNT };
     struct AssertionDsc
     {
         optAssertionKind        assertionKind;
@@ -5754,6 +5757,10 @@ public:
             case O2K_INVALID:
                 // we will return false
                 break;
+
+            default:
+                assert(!"Unexpected value for op2.kind in AssertionDsc.");
+                break;
             }
             return false;
         }
@@ -5892,6 +5899,7 @@ public :
 
 #ifdef DEBUG
     void optPrintAssertion(AssertionDsc*  newAssertion, AssertionIndex assertionIndex=0);
+    void optDebugCheckAssertion(AssertionDsc* assertion);
     void optDebugCheckAssertions(AssertionIndex AssertionIndex);
 #endif
     void optAddCopies();
diff --git a/src/jit/compiler.hpp b/src/jit/compiler.hpp
index b6127e72ed..1ff7b53348 100644
--- a/src/jit/compiler.hpp
+++ b/src/jit/compiler.hpp
@@ -3474,8 +3474,9 @@ void                Compiler::optAssertionReset(AssertionIndex limit)
 
     while (optAssertionCount > limit)
     {
-        AssertionIndex index  = optAssertionCount--;
+        AssertionIndex index  = optAssertionCount;
         AssertionDsc* curAssertion = optGetAssertion(index);
+        optAssertionCount--;
         unsigned lclNum = curAssertion->op1.lcl.lclNum;
         assert(lclNum < lvaTableCnt);
         BitVecOps::RemoveElemD(apTraits, GetAssertionDep(lclNum), index - 1);