Add/improve assertion propagation comments

Commit migrated from https://github.com/dotnet/coreclr/commit/f81e625c6a030436e3e50d699e44f529d797997c

diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp
index cf7190b..192ed5c 100644 (file)
--- a/src/coreclr/src/jit/assertionprop.cpp
+++ b/src/coreclr/src/jit/assertionprop.cpp
@@ -4471,6 +4471,7 @@ ASSERT_TP* Compiler::optComputeAssertionGen()
             {
                 if (tree->gtOper == GT_JTRUE)
                 {
+                    // A GT_TRUE is always the last node in a tree, so we can break here
                     assert((tree->gtNext == nullptr) && (stmt->gtNext == nullptr));
                     jtrue = tree;
                     break;
@@ -4498,17 +4499,19 @@ ASSERT_TP* Compiler::optComputeAssertionGen()
                     index &= OAE_INDEX_MASK;
                     // Currently OAE_NEXT_EDGE is only used with OAK_NO_THROW assertions
                     assert(optGetAssertion(index)->assertionKind == OAK_NO_THROW);
-                    // Don't bother with implied/complementary assertions, there aren't any for OAK_NO_THROW
+                    // Don't bother with implied assertions, there aren't any for OAK_NO_THROW
                     BitVecOps::AddElemD(apTraits, valueGen, index - 1);
                 }
                 else
                 {
+                    // If GT_JTRUE, and true path, update jumpDestValueGen.
                     optImpliedAssertions(index, jumpDestValueGen);
                     BitVecOps::AddElemD(apTraits, jumpDestValueGen, index - 1);
 
                     index = optFindComplementary(index);
                     if (index != NO_ASSERTION_INDEX)
                     {
+                        // If GT_JTRUE, and false path and we have a complementary assertion available update valueGen
                         optImpliedAssertions(index, valueGen);
                         BitVecOps::AddElemD(apTraits, valueGen, index - 1);
                     }
@@ -5117,6 +5120,7 @@ void Compiler::optAssertionPropMain()
             {
                 if (tree->OperIs(GT_JTRUE))
                 {
+                    // A GT_TRUE is always the last node in a tree, so we can break here
                     assert((tree->gtNext == nullptr) && (stmt->gtNext == nullptr));
                     break;
                 }

diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h
index 2f96b85..be71875 100644 (file)
--- a/src/coreclr/src/jit/compiler.h
+++ b/src/coreclr/src/jit/compiler.h
@@ -5882,6 +5882,12 @@ public:
 
     typedef unsigned short AssertionIndex;
 
+    // By default the assertion generated by a GT_JTRUE node holds on the true (bbJumpDest) edge.
+    // If the OAE_NEXT_EDGE bit of the assertion index is set then the assertion holds on the false (bbNext) edge
+    // and the OAE_NEXT_EDGE bit needs to be masked to obtain the real assertion index.
+    // Currently this is used by OAK_NO_THROW assertions but it may also be useful for other kinds of assertions
+    // by removing the need to create unnecessary complementary assertions. However, this bit twiddling mechanism
+    // is fragile and should be replaced with something cleaner (e.g. struct + bitfield).
     enum optAssertionEdge : AssertionIndex
     {
         // OAE_JUMP_EDGE  = 0x0000, // assertion holds for bbJumpDest (default)

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
index cd1034d..0ef02d1 100644 (file)
--- a/src/coreclr/src/jit/gentree.h
+++ b/src/coreclr/src/jit/gentree.h
@@ -397,6 +397,7 @@ struct GenTree
 
 #if ASSERTION_PROP
     unsigned short gtAssertionNum; // 0 or Assertion table index
+                                   // possibly ORed with optAssertionEdge::OAE_NEXT_EDGE
                                    // valid only for non-GT_STMT nodes
 
     bool HasAssertion() const

diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp
index 03cfb5b..b771bf6 100644 (file)
--- a/src/coreclr/src/jit/valuenum.cpp
+++ b/src/coreclr/src/jit/valuenum.cpp
@@ -3164,15 +3164,17 @@ void ValueNumStore::GetConstantBoundInfo(ValueNum vn, ConstantBoundInfo* info)
 }
 
 //------------------------------------------------------------------------
-// IsVNArrLenUnsignedBound: Checks if the specified vn represents an
-//    expression such as "(uint)i < (uint)a.len" that imply that the
-//    array index is valid (0 <= i && i < a.len).
+// IsVNArrLenUnsignedBound: Checks if the specified vn represents an expression
+//    such as "(uint)i < (uint)a.len" that implies that the array index is valid
+//    (0 <= i && i < a.len).
 //
 // Arguments:
 //    vn - Value number to query
-//    info - Pointer to a ArrLenUnsignedBoundInfo object to return
-//           information about the expression. Not populated if the
-//           vn expression isn't suitable (e.g. i <= a.len)
+//    info - Pointer to an ArrLenUnsignedBoundInfo object to return information about
+//           the expression. Not populated if the vn expression isn't suitable (e.g. i <= a.len).
+//           This enables optCreateJTrueBoundAssertion to immediatly create an OAK_NO_THROW
+//           assertion instead of the OAK_EQUAL/NOT_EQUAL assertions created by signed compares
+//           (IsVNArrLenBound, IsVNArrLenArithBound) that require further processing.
 
 bool ValueNumStore::IsVNArrLenUnsignedBound(ValueNum vn, ArrLenUnsignedBoundInfo* info)
 {