JIT: Improve interference checks for thrown exceptions and enable CSE/hoisting of GVM resolution helpers

src/coreclr/jit/compiler.h

@@ -5906,6 +5906,9 @@ class Compiler
     // Adds the exception set for the current tree node which has a memory indirection operation
     void fgValueNumberAddExceptionSetForIndirection(GenTree* tree, GenTree* baseAddr);
 
+    // Create VNP for NullPtrExc for something that null checks a base address
+    ValueNumPair fgValueNumberIndirNullCheckExceptions(GenTree* baseAddr);
+
     // Adds the exception sets for the current tree node which is performing a division or modulus operation
     void fgValueNumberAddExceptionSetForDivision(GenTree* tree);
 

src/coreclr/jit/forwardsub.cpp

@@ -257,16 +257,13 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>
         }
 
         m_accumulatedFlags |= (node->gtFlags & GTF_GLOB_EFFECT);
-        if ((node->gtFlags & GTF_CALL) != 0)
+        if ((node->gtFlags & GTF_EXCEPT) != 0)
         {
-            m_accumulatedExceptions = ExceptionSetFlags::All;
-        }
-        else if ((node->gtFlags & GTF_EXCEPT) != 0)
-        {
-            // We can never reorder in the face of different exception types,
-            // so stop calling 'OperExceptions' once we've seen more than one
-            // different exception type.
-            if (genCountBits(static_cast<uint32_t>(m_accumulatedExceptions)) <= 1)
+            // We can never reorder in the face of different or unknown
+            // exception types, so stop calling 'OperExceptions' once we've
+            // seen more than one different exception type.
+            if ((genCountBits(static_cast<uint32_t>(m_accumulatedExceptions)) <= 1) &&
+                ((m_accumulatedExceptions & ExceptionSetFlags::UnknownException) == ExceptionSetFlags::None))
             {
                 m_accumulatedExceptions |= node->OperExceptions(m_compiler);
             }
@@ -694,9 +691,10 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)
         if ((fsv.GetFlags() & GTF_EXCEPT) != 0)
         {
             assert(fsv.GetExceptions() != ExceptionSetFlags::None);
-            if (genCountBits(static_cast<uint32_t>(fsv.GetExceptions())) > 1)
+            if ((genCountBits(static_cast<uint32_t>(fsv.GetExceptions())) > 1) ||
+                (((fsv.GetExceptions() & ExceptionSetFlags::UnknownException) != ExceptionSetFlags::None)))
             {
-                JITDUMP(" cannot reorder different thrown exceptions\n");
+                JITDUMP(" cannot reorder different/unknown thrown exceptions\n");
                 return false;
             }
 

src/coreclr/jit/gentree.cpp

@@ -7183,8 +7183,9 @@ bool GenTree::OperIsImplicitIndir() const
 //    A bit set of exceptions this tree may throw.
 //
 // Remarks:
-//    Should not be used on calls given that we can say nothing precise about
-//    those.
+//   The ExceptionSetFlags::UnknownException must generally be handled
+//   specially by the consumer; when it is present it means we can say nothing
+//   precise about the thrown exceptions.
 //
 ExceptionSetFlags GenTree::OperExceptions(Compiler* comp)
 {
@@ -7236,9 +7237,14 @@ ExceptionSetFlags GenTree::OperExceptions(Compiler* comp)
             return ExceptionSetFlags::None;
 
         case GT_CALL:
-            assert(!"Unexpected GT_CALL in OperExceptions");
-            return ExceptionSetFlags::All;
+            CorInfoHelpFunc helper;
+            helper = comp->eeGetHelperNum(this->AsCall()->gtCallMethHnd);
+            if (helper == CORINFO_HELP_UNDEF)
+            {
+                return ExceptionSetFlags::UnknownException;
+            }
 
+            return Compiler::s_helperCallProperties.ThrownExceptions(helper);
         case GT_LOCKADD:
         case GT_XAND:
         case GT_XORR:
@@ -7286,7 +7292,10 @@ ExceptionSetFlags GenTree::OperExceptions(Compiler* comp)
 #ifdef FEATURE_HW_INTRINSICS
         case GT_HWINTRINSIC:
         {
-            assert((gtFlags & GTF_HW_USER_CALL) == 0);
+            if ((gtFlags & GTF_HW_USER_CALL) != 0)
+            {
+                return ExceptionSetFlags::UnknownException;
+            }
 
             GenTreeHWIntrinsic* hwIntrinsicNode = this->AsHWIntrinsic();
 
@@ -7332,32 +7341,6 @@ ExceptionSetFlags GenTree::OperExceptions(Compiler* comp)
 //
 bool GenTree::OperMayThrow(Compiler* comp)
 {
-    if (OperIs(GT_CALL))
-    {
-        CorInfoHelpFunc helper;
-        helper = comp->eeGetHelperNum(this->AsCall()->gtCallMethHnd);
-        return ((helper == CORINFO_HELP_UNDEF) || !comp->s_helperCallProperties.NoThrow(helper));
-    }
-#ifdef FEATURE_HW_INTRINSICS
-    else if (OperIsHWIntrinsic())
-    {
-        if ((gtFlags & GTF_HW_USER_CALL) != 0)
-        {
-            return true;
-        }
-
-#ifdef TARGET_XARCH
-        NamedIntrinsic intrinsicId = this->AsHWIntrinsic()->GetHWIntrinsicId();
-        if (intrinsicId == NI_Vector128_op_Division || intrinsicId == NI_Vector256_op_Division ||
-            intrinsicId == NI_Vector512_op_Division)
-        {
-            assert(varTypeIsInt(AsHWIntrinsic()->GetSimdBaseType()));
-            return true;
-        }
-#endif // TARGET_XARCH
-    }
-#endif // FEATURE_HW_INTRINSICS
-
     return OperExceptions(comp) != ExceptionSetFlags::None;
 }
 
@@ -7513,12 +7496,13 @@ bool GenTree::OperSupportsOrderingSideEffect() const
 // excluding its children.
 //
 // Arguments:
-//    comp -  Compiler instance
+//    comp              - Compiler instance
+//    preciseExceptions - [out] Precise exceptions this node may throw
 //
 // Return Value:
 //    The effect flags.
 //
-GenTreeFlags GenTree::OperEffects(Compiler* comp)
+GenTreeFlags GenTree::OperEffects(Compiler* comp, ExceptionSetFlags* preciseExceptions)
 {
     GenTreeFlags flags = gtFlags & GTF_ALL_EFFECT;
 
@@ -7532,9 +7516,17 @@ GenTreeFlags GenTree::OperEffects(Compiler* comp)
         flags &= ~GTF_CALL;
     }
 
-    if (((flags & GTF_EXCEPT) != 0) && !OperMayThrow(comp))
+    if ((flags & GTF_EXCEPT) != 0)
     {
-        flags &= ~GTF_EXCEPT;
+        *preciseExceptions = OperExceptions(comp);
+        if (*preciseExceptions == ExceptionSetFlags::None)
+        {
+            flags &= ~GTF_EXCEPT;
+        }
+    }
+    else
+    {
+        *preciseExceptions = ExceptionSetFlags::None;
     }
 
     if (((flags & GTF_GLOB_REF) != 0) && !OperRequiresGlobRefFlag(comp))
@@ -17908,11 +17900,6 @@ ExceptionSetFlags Compiler::gtCollectExceptions(GenTree* tree)
         }
     };
 
-    // We only expect the caller to ask for precise exceptions for cases where
-    // it may help with disambiguating between exceptions. If the tree contains
-    // a call it can always throw arbitrary exceptions.
-    assert((tree->gtFlags & GTF_CALL) == 0);
-
     ExceptionsWalker walker(this);
     walker.WalkTree(&tree, nullptr);
     assert(((tree->gtFlags & GTF_EXCEPT) == 0) || (walker.GetFlags() != ExceptionSetFlags::None));

src/coreclr/jit/gentree.h

@@ -116,20 +116,6 @@ enum gtCallTypes : BYTE
     CT_COUNT // fake entry (must be last)
 };
 
-enum class ExceptionSetFlags : uint32_t
-{
-    None                     = 0x0,
-    OverflowException        = 0x1,
-    DivideByZeroException    = 0x2,
-    ArithmeticException      = 0x4,
-    NullReferenceException   = 0x8,
-    IndexOutOfRangeException = 0x10,
-    StackOverflowException   = 0x20,
-
-    All = OverflowException | DivideByZeroException | ArithmeticException | NullReferenceException |
-          IndexOutOfRangeException | StackOverflowException,
-};
-
 inline constexpr ExceptionSetFlags operator~(ExceptionSetFlags a)
 {
     return (ExceptionSetFlags)(~(uint32_t)a);
@@ -1939,7 +1925,12 @@ struct GenTree
 
     bool OperSupportsOrderingSideEffect() const;
 
-    GenTreeFlags OperEffects(Compiler* comp);
+    GenTreeFlags OperEffects(Compiler* comp, ExceptionSetFlags* preciseExceptions);
+    GenTreeFlags OperEffects(Compiler* comp)
+    {
+        ExceptionSetFlags preciseExceptions;
+        return OperEffects(comp, &preciseExceptions);
+    }
 
     unsigned GetScaleIndexMul();
     unsigned GetScaleIndexShf();

src/coreclr/jit/lower.cpp

@@ -6306,15 +6306,12 @@ void Lowering::OptimizeCallIndirectTargetEvaluation(GenTreeCall* call)
 {
     assert((call->gtCallType == CT_INDIRECT) && (call->gtCallAddr != nullptr));
 
-    if (!call->gtCallAddr->IsHelperCall(comp, CORINFO_HELP_VIRTUAL_FUNC_PTR) &&
-        !call->gtCallAddr->IsHelperCall(comp, CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR) &&
-        !call->gtCallAddr->IsHelperCall(comp, CORINFO_HELP_GVMLOOKUP_FOR_SLOT) &&
-        !call->gtCallAddr->IsHelperCall(comp, CORINFO_HELP_READYTORUN_GENERIC_HANDLE))
+    if (!call->gtCallAddr->IsCall())
     {
         return;
     }
 
-    JITDUMP("Target is a GVM; seeing if we can move arguments ahead of resolution\n");
+    JITDUMP("Indirect call target is itself a call; trying to reorder its evaluation with arguments\n");
 
     m_scratchSideEffects.Clear();
 
@@ -6357,9 +6354,9 @@ void Lowering::OptimizeCallIndirectTargetEvaluation(GenTreeCall* call)
 
         if (cur == call->gtCallAddr)
         {
-            // Start moving this range. Do not add its side effects as we will
-            // check the NRE manually for precision.
+            // Start moving this range.
             movingRange = LIR::ReadOnlyRange(cur, cur);
+            m_scratchSideEffects.AddNode(comp, cur);
             continue;
         }
 
@@ -6374,41 +6371,12 @@ void Lowering::OptimizeCallIndirectTargetEvaluation(GenTreeCall* call)
         {
             // This node is in the dataflow. See if we can move it ahead of the
             // range we are moving.
-            bool interferes = false;
             if (m_scratchSideEffects.InterferesWith(comp, cur, /* strict */ true))
-            {
-                JITDUMP("  Stopping at [%06u]; it interferes with the current range we are moving\n",
-                        Compiler::dspTreeID(cur));
-                interferes = true;
-            }
-
-            if (!interferes)
-            {
-                // No problem so far. However the side effect set does not
-                // include the GVM call itself, which can throw NRE. Check the
-                // NRE now for precision.
-                GenTreeFlags flags = cur->OperEffects(comp);
-                if ((flags & GTF_PERSISTENT_SIDE_EFFECTS) != 0)
-                {
-                    JITDUMP("  Stopping at [%06u]; it has persistent side effects\n", Compiler::dspTreeID(cur));
-                    interferes = true;
-                }
-                else if ((flags & GTF_EXCEPT) != 0)
-                {
-                    ExceptionSetFlags preciseExceptions = cur->OperExceptions(comp);
-                    if (preciseExceptions != ExceptionSetFlags::NullReferenceException)
-                    {
-                        JITDUMP("  Stopping at [%06u]; it throws an exception that is not NRE\n",
-                                Compiler::dspTreeID(cur));
-                        interferes = true;
-                    }
-                }
-            }
-
-            if (interferes)
             {
                 // Stop moving the range, but keep going through the rest
                 // of the nodes to unmark them
+                JITDUMP("  Stopping at [%06u]; it interferes with the current range we are moving\n",
+                        Compiler::dspTreeID(cur));
                 movingRange = LIR::ReadOnlyRange();
             }
             else

src/coreclr/jit/morph.cpp

@@ -911,8 +911,10 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call)
                     exceptionFlags = comp->gtCollectExceptions(argx);
                 }
 
-                bool exactlyOne       = isPow2(static_cast<unsigned>(exceptionFlags));
-                bool throwsSameAsPrev = exactlyOne && (exceptionFlags == prevExceptionFlags);
+                bool exactlyOneKnown =
+                    isPow2(static_cast<unsigned>(exceptionFlags)) &&
+                    ((exceptionFlags & ExceptionSetFlags::UnknownException) == ExceptionSetFlags::None);
+                bool throwsSameAsPrev = exactlyOneKnown && (exceptionFlags == prevExceptionFlags);
                 if (!throwsSameAsPrev)
                 {
                     JITDUMP("Exception set for arg [%06u] interferes with previous tree [%06u]; must evaluate previous "
@@ -5664,10 +5666,11 @@ GenTree* Compiler::getVirtMethodPointerTree(GenTree*                thisPtr,
                                             CORINFO_RESOLVED_TOKEN* pResolvedToken,
                                             CORINFO_CALL_INFO*      pCallInfo)
 {
-    GenTree* exactTypeDesc   = getTokenHandleTree(pResolvedToken, true);
     GenTree* exactMethodDesc = getTokenHandleTree(pResolvedToken, false);
+    GenTree* exactTypeDesc   = getTokenHandleTree(pResolvedToken, true);
 
-    return gtNewHelperCallNode(CORINFO_HELP_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr, exactTypeDesc, exactMethodDesc);
+    return gtNewVirtualFunctionLookupHelperCallNode(CORINFO_HELP_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr, exactMethodDesc,
+                                                    exactTypeDesc);
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/sideeffects.cpp

@@ -489,7 +489,10 @@ SideEffectSet::SideEffectSet(Compiler* compiler, GenTree* node)
 //
 void SideEffectSet::AddNode(Compiler* compiler, GenTree* node)
 {
-    m_sideEffectFlags |= node->OperEffects(compiler);
+    ExceptionSetFlags preciseExceptions;
+    GenTreeFlags      operEffects = node->OperEffects(compiler, &preciseExceptions);
+    m_sideEffectFlags |= operEffects;
+    m_preciseExceptions |= preciseExceptions;
     m_aliasSet.AddNode(compiler, node);
 }
 
@@ -507,12 +510,14 @@ void SideEffectSet::AddNode(Compiler* compiler, GenTree* node)
 //        - One set's reads and writes interfere with the other set's reads and writes
 //
 // Arguments:
-//    otherSideEffectFlags - The side effect flags for the other side effect set.
+//    otherSideEffectFlags   - The side effect flags for the other side effect set.
+//    otherPreciseExceptions - The precise exceptions for the other side effect set.
 //    otherAliasInfo - The alias information for the other side effect set.
 //    strict - True if the analysis should be strict as described above.
 //
 template <typename TOtherAliasInfo>
 bool SideEffectSet::InterferesWith(unsigned               otherSideEffectFlags,
+                                   ExceptionSetFlags      otherPreciseExceptions,
                                    const TOtherAliasInfo& otherAliasInfo,
                                    bool                   strict) const
 {
@@ -535,10 +540,15 @@ bool SideEffectSet::InterferesWith(unsigned               otherSideEffectFlags,
             return true;
         }
 
-        // If both sets produce an exception, the sets interfere.
+        // If both sets produce non-reorderable exceptions the sets interfere
         if (thisProducesException && otherProducesException)
         {
-            return true;
+            if ((((m_preciseExceptions | otherPreciseExceptions) & ExceptionSetFlags::UnknownException) !=
+                 ExceptionSetFlags::None) ||
+                (genCountBits((uint32_t)m_preciseExceptions) > 1) || (m_preciseExceptions != otherPreciseExceptions))
+            {
+                return true;
+            }
         }
     }
 
@@ -575,7 +585,7 @@ bool SideEffectSet::InterferesWith(unsigned               otherSideEffectFlags,
 //
 bool SideEffectSet::InterferesWith(const SideEffectSet& other, bool strict) const
 {
-    return InterferesWith(other.m_sideEffectFlags, other.m_aliasSet, strict);
+    return InterferesWith(other.m_sideEffectFlags, other.m_preciseExceptions, other.m_aliasSet, strict);
 }
 
 //------------------------------------------------------------------------
@@ -593,7 +603,9 @@ bool SideEffectSet::InterferesWith(const SideEffectSet& other, bool strict) cons
 //
 bool SideEffectSet::InterferesWith(Compiler* compiler, GenTree* node, bool strict) const
 {
-    return InterferesWith(node->OperEffects(compiler), AliasSet::NodeInfo(compiler, node), strict);
+    ExceptionSetFlags preciseExceptions;
+    GenTreeFlags      operEffects = node->OperEffects(compiler, &preciseExceptions);
+    return InterferesWith(operEffects, preciseExceptions, AliasSet::NodeInfo(compiler, node), strict);
 }
 
 //------------------------------------------------------------------------
@@ -782,6 +794,7 @@ bool SideEffectSet::IsLirRangeInvariantInRange(
 //
 void SideEffectSet::Clear()
 {
-    m_sideEffectFlags = 0;
+    m_sideEffectFlags   = 0;
+    m_preciseExceptions = ExceptionSetFlags::None;
     m_aliasSet.Clear();
 }

src/coreclr/jit/sideeffects.h

@@ -168,11 +168,15 @@ class AliasSet final
 //
 class SideEffectSet final
 {
-    unsigned m_sideEffectFlags; // A mask of GTF_* flags that represents exceptional and barrier side effects.
-    AliasSet m_aliasSet;        // An AliasSet that represents read and write side effects.
+    unsigned          m_sideEffectFlags; // A mask of GTF_* flags that represents exceptional and barrier side effects.
+    ExceptionSetFlags m_preciseExceptions; // Set representing exceptions that may be thrown
+    AliasSet          m_aliasSet;          // An AliasSet that represents read and write side effects.
 
     template <typename TOtherAliasInfo>
-    bool InterferesWith(unsigned otherSideEffectFlags, const TOtherAliasInfo& otherAliasInfo, bool strict) const;
+    bool InterferesWith(unsigned               otherSideEffectFlags,
+                        ExceptionSetFlags      otherPreciseExceptions,
+                        const TOtherAliasInfo& otherAliasInfo,
+                        bool                   strict) const;
 
 public:
     SideEffectSet();