Improve value numbering for casts (#59841)

* Refactor how casts are VNed, part 1

Remove duplication between VNPairForCast
and VNForCast, route the former through
the latter.

Also change the debug output for casts.

No diffs for this commit.

* Refactor how casts are VNed, part 2

VN cast helpers just like casts.
This allows them to be folded.

In the past, this was dangerous because some of
the pessimization protected us from undefined
behavior in cases of out-of-bounds conversions,
but now we do not fold such cases anymore.

* Enable VN-based constant propagation for calls

This solves some of the regressions seen due to
now-missing CSEs of these helper calls evaluated
as constants by VN.

* Fix VNFuncs for MUL_OVF helpers

This fixes the test failures revealed by
the more aggressive constant propagation.

This is very similar to a bug that was recently
fixed where checked casts were treated as non-checked
and could be observed without the assertion propagation
changes as well, with the redundant branches optimization.

src/coreclr/jit/assertionprop.cpp

@@ -5410,7 +5410,14 @@ GenTree* Compiler::optExtractSideEffListFromConst(GenTree* tree)
     {
         // Do a sanity check to ensure persistent side effects aren't discarded and
         // tell gtExtractSideEffList to ignore the root of the tree.
-        assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS));
+        // We are relying here on an invariant that VN will only fold non-throwing expressions.
+        const bool ignoreExceptions = true;
+        const bool ignoreCctors     = false;
+        // We have to check "AsCall()->HasSideEffects()" here separately because "gtNodeHasSideEffects"
+        // also checks for side effects that arguments introduce (incosistently so, it otherwise only
+        // checks for the side effects the node itself has). TODO-Cleanup: change it to not do that?
+        assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS) ||
+               (tree->IsCall() && !tree->AsCall()->HasSideEffects(this, ignoreExceptions, ignoreCctors)));
 
         // Exception side effects may be ignored because the root is known to be a constant
         // (e.g. VN may evaluate a DIV/MOD node to a constant and the node may still
@@ -5610,6 +5617,13 @@ Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, Sta
             }
             break;
 
+        case GT_CALL:
+            if (!tree->AsCall()->IsPure(this))
+            {
+                return WALK_CONTINUE;
+            }
+            break;
+
         default:
             // Unknown node, continue to walk.
             return WALK_CONTINUE;

src/coreclr/jit/compiler.h

@@ -5484,6 +5484,9 @@ class Compiler
     // Does value-numbering for a call.  We interpret some helper calls.
     void fgValueNumberCall(GenTreeCall* call);
 
+    // Does value-numbering for a helper representing a cast operation.
+    void fgValueNumberCastHelper(GenTreeCall* call);
+
     // Does value-numbering for a helper "call" that has a VN function symbol "vnf".
     void fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueNumPair vnpExc);
 

src/coreclr/jit/gentree.cpp

@@ -15908,7 +15908,7 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
             {
                 if (m_compiler->gtNodeHasSideEffects(node, m_flags))
                 {
-                    m_sideEffects.Push(node);
+                    PushSideEffects(node);
                     if (node->OperIsBlk() && !node->OperIsStoreBlk())
                     {
                         JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
@@ -15925,7 +15925,7 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
                 // gtNodeHasSideEffects and make this check unconditionally.
                 if (node->OperIsAtomicOp())
                 {
-                    m_sideEffects.Push(node);
+                    PushSideEffects(node);
                     return Compiler::WALK_SKIP_SUBTREES;
                 }
 
@@ -15936,7 +15936,7 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
                         (node->gtGetOp1()->TypeGet() == TYP_STRUCT))
                     {
                         JITDUMP("Keep the GT_ADDR and GT_IND together:\n");
-                        m_sideEffects.Push(node);
+                        PushSideEffects(node);
                         return Compiler::WALK_SKIP_SUBTREES;
                     }
                 }
@@ -15953,7 +15953,7 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
                 // those need to be extracted as if they're side effects.
                 if (!UnmarkCSE(node))
                 {
-                    m_sideEffects.Push(node);
+                    PushSideEffects(node);
                     return Compiler::WALK_SKIP_SUBTREES;
                 }
 
@@ -15989,6 +15989,16 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
                 return false;
             }
         }
+
+        void PushSideEffects(GenTree* node)
+        {
+            // The extracted side effect will no longer be an argument, so unmark it.
+            // This is safe to do because the side effects will be visited in pre-order,
+            // aborting as soon as any tree is extracted. Thus if an argument for a call
+            // is being extracted, it is guaranteed that the call itself will not be.
+            node->gtFlags &= ~GTF_LATE_ARG;
+            m_sideEffects.Push(node);
+        }
     };
 
     SideEffectExtractor extractor(this, flags);