diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ad6314f7339e9b..df9fcec6a78ab3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5376,13 +5376,6 @@ class Compiler void* pCallBackData = nullptr, bool computeStack = false); - // An fgWalkPreFn that looks for expressions that have inline throws in - // minopts mode. Basically it looks for tress with gtOverflowEx() or - // GTF_IND_RNGCHK. It returns WALK_ABORT if one is found. It - // returns WALK_SKIP_SUBTREES if GTF_EXCEPT is not set (assumes flags - // properly propagated to parent trees). It returns WALK_CONTINUE - // otherwise. - static fgWalkResult fgChkThrowCB(GenTree** pTree, Compiler::fgWalkData* data); static fgWalkResult fgChkLocAllocCB(GenTree** pTree, Compiler::fgWalkData* data); static fgWalkResult fgChkQmarkCB(GenTree** pTree, Compiler::fgWalkData* data); @@ -5899,6 +5892,8 @@ class Compiler bool gtIsTypeHandleToRuntimeTypeHandleHelper(GenTreeCall* call, CorInfoHelpFunc* pHelper = nullptr); bool gtIsActiveCSE_Candidate(GenTree* tree); + ExceptionSetFlags gtCollectExceptions(GenTree* tree); + bool fgIsBigOffset(size_t offset); bool fgNeedReturnSpillTemp(); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index ac240be9a2d27f..33d6fab81e5aef 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4130,47 +4130,6 @@ void Compiler::fgSetBlockOrder(BasicBlock* block) return firstNode; } -/*static*/ Compiler::fgWalkResult Compiler::fgChkThrowCB(GenTree** pTree, fgWalkData* data) -{ - GenTree* tree = *pTree; - - // If this tree doesn't have the EXCEPT flag set, then there is no - // way any of the child nodes could throw, so we can stop recursing. - if (!(tree->gtFlags & GTF_EXCEPT)) - { - return Compiler::WALK_SKIP_SUBTREES; - } - - switch (tree->gtOper) - { - case GT_MUL: - case GT_ADD: - case GT_SUB: - case GT_CAST: - if (tree->gtOverflow()) - { - return Compiler::WALK_ABORT; - } - break; - - case GT_INDEX_ADDR: - // This calls CORINFO_HELP_RNGCHKFAIL for Debug code. - if (tree->AsIndexAddr()->IsBoundsChecked()) - { - return Compiler::WALK_ABORT; - } - break; - - case GT_BOUNDS_CHECK: - return Compiler::WALK_ABORT; - - default: - break; - } - - return Compiler::WALK_CONTINUE; -} - /*static*/ Compiler::fgWalkResult Compiler::fgChkLocAllocCB(GenTree** pTree, fgWalkData* data) { GenTree* tree = *pTree; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 818a969122fea7..17842a4195f029 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6354,16 +6354,20 @@ bool GenTree::OperIsImplicitIndir() const } //------------------------------------------------------------------------------ -// OperMayThrow : Check whether the operation may throw. +// OperExceptions : Get exception set this tree may throw. // // // Arguments: // comp - Compiler instance // // Return Value: -// True if the given operator may cause an exception - -bool GenTree::OperMayThrow(Compiler* comp) +// 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. +// +ExceptionSetFlags GenTree::OperExceptions(Compiler* comp) { GenTree* op; @@ -6380,26 +6384,40 @@ bool GenTree::OperMayThrow(Compiler* comp) if (varTypeIsFloating(op->TypeGet())) { - return false; // Floating point division does not throw. + return ExceptionSetFlags::None; } // For integers only division by 0 or by -1 can throw - if (op->IsIntegralConst() && !op->IsIntegralConst(0) && !op->IsIntegralConst(-1)) + if (op->IsIntegralConst()) { - return false; + if (op->IsIntegralConst(0)) + { + return ExceptionSetFlags::DivideByZeroException; + } + if (op->IsIntegralConst(-1)) + { + return ExceptionSetFlags::ArithmeticException; + } + + return ExceptionSetFlags::None; } - return true; + + return ExceptionSetFlags::DivideByZeroException | ExceptionSetFlags::ArithmeticException; case GT_INTRINSIC: // If this is an intrinsic that represents the object.GetType(), it can throw an NullReferenceException. // Currently, this is the only intrinsic that can throw an exception. - return AsIntrinsic()->gtIntrinsicName == NI_System_Object_GetType; + if (AsIntrinsic()->gtIntrinsicName == NI_System_Object_GetType) + { + return ExceptionSetFlags::NullReferenceException; + } + + return ExceptionSetFlags::None; case GT_CALL: + assert(!"Unexpected GT_CALL in OperExceptions"); - CorInfoHelpFunc helper; - helper = comp->eeGetHelperNum(this->AsCall()->gtCallMethHnd); - return ((helper == CORINFO_HELP_UNDEF) || !comp->s_helperCallProperties.NoThrow(helper)); + return ExceptionSetFlags::All; case GT_IND: case GT_BLK: @@ -6407,14 +6425,28 @@ bool GenTree::OperMayThrow(Compiler* comp) case GT_NULLCHECK: case GT_STORE_BLK: case GT_STORE_DYN_BLK: - return (((this->gtFlags & GTF_IND_NONFAULTING) == 0) && comp->fgAddrCouldBeNull(this->AsIndir()->Addr())); + if (((this->gtFlags & GTF_IND_NONFAULTING) == 0) && comp->fgAddrCouldBeNull(this->AsIndir()->Addr())) + { + return ExceptionSetFlags::NullReferenceException; + } + + return ExceptionSetFlags::None; case GT_ARR_LENGTH: - return (((this->gtFlags & GTF_IND_NONFAULTING) == 0) && - comp->fgAddrCouldBeNull(this->AsArrLen()->ArrRef())); + if (((this->gtFlags & GTF_IND_NONFAULTING) == 0) && comp->fgAddrCouldBeNull(this->AsArrLen()->ArrRef())) + { + return ExceptionSetFlags::NullReferenceException; + } + + return ExceptionSetFlags::None; case GT_ARR_ELEM: - return comp->fgAddrCouldBeNull(this->AsArrElem()->gtArrObj); + if (comp->fgAddrCouldBeNull(this->AsArrElem()->gtArrObj)) + { + return ExceptionSetFlags::NullReferenceException; + } + + return ExceptionSetFlags::None; case GT_FIELD: { @@ -6422,19 +6454,28 @@ bool GenTree::OperMayThrow(Compiler* comp) if (fldObj != nullptr) { - return comp->fgAddrCouldBeNull(fldObj); + if (comp->fgAddrCouldBeNull(fldObj)) + { + return ExceptionSetFlags::NullReferenceException; + } } - return false; + return ExceptionSetFlags::None; } case GT_BOUNDS_CHECK: + case GT_INDEX_ADDR: + return ExceptionSetFlags::IndexOutOfRangeException; + case GT_ARR_INDEX: case GT_ARR_OFFSET: - case GT_LCLHEAP: + return ExceptionSetFlags::NullReferenceException; + case GT_CKFINITE: - case GT_INDEX_ADDR: - return true; + return ExceptionSetFlags::ArithmeticException; + + case GT_LCLHEAP: + return ExceptionSetFlags::StackOverflowException; #ifdef FEATURE_HW_INTRINSICS case GT_HWINTRINSIC: @@ -6446,23 +6487,42 @@ bool GenTree::OperMayThrow(Compiler* comp) // This operation contains an implicit indirection // it could throw a null reference exception. // - return true; + return ExceptionSetFlags::NullReferenceException; } - break; + + return ExceptionSetFlags::None; } #endif // FEATURE_HW_INTRINSICS default: - break; - } + if (gtOverflowEx()) + { + return ExceptionSetFlags::OverflowException; + } - /* Overflow arithmetic operations also throw exceptions */ + return ExceptionSetFlags::None; + } +} - if (gtOverflowEx()) +//------------------------------------------------------------------------------ +// OperMayThrow : Check whether the operation may throw. +// +// +// Arguments: +// comp - Compiler instance +// +// Return Value: +// True if the given operator may cause an exception +// +bool GenTree::OperMayThrow(Compiler* comp) +{ + if (OperIs(GT_CALL)) { - return true; + CorInfoHelpFunc helper; + helper = comp->eeGetHelperNum(this->AsCall()->gtCallMethHnd); + return ((helper == CORINFO_HELP_UNDEF) || !comp->s_helperCallProperties.NoThrow(helper)); } - return false; + return OperExceptions(comp) != ExceptionSetFlags::None; } //----------------------------------------------------------------------------------- @@ -16116,7 +16176,7 @@ bool Compiler::gtIsTypeHandleToRuntimeTypeHelper(GenTreeCall* call) // // Return Value: // True if so - +// bool Compiler::gtIsTypeHandleToRuntimeTypeHandleHelper(GenTreeCall* call, CorInfoHelpFunc* pHelper) { CorInfoHelpFunc helper = CORINFO_HELP_UNDEF; @@ -16143,6 +16203,61 @@ bool Compiler::gtIsActiveCSE_Candidate(GenTree* tree) return (optValnumCSE_phase && IS_CSE_INDEX(tree->gtCSEnum)); } +//------------------------------------------------------------------------ +// gtCollectExceptions: walk a tree collecting a bit set of exceptions the tree +// may throw. +// +// Arguments: +// tree - tree to examine +// +// Return Value: +// Bit set of exceptions the tree may throw. +// +ExceptionSetFlags Compiler::gtCollectExceptions(GenTree* tree) +{ + class ExceptionsWalker final : public GenTreeVisitor + { + ExceptionSetFlags m_preciseExceptions = ExceptionSetFlags::None; + + public: + ExceptionsWalker(Compiler* comp) : GenTreeVisitor(comp) + { + } + + enum + { + DoPreOrder = true, + }; + + ExceptionSetFlags GetFlags() + { + return m_preciseExceptions; + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* tree = *use; + if ((tree->gtFlags & GTF_EXCEPT) == 0) + { + return WALK_SKIP_SUBTREES; + } + + m_preciseExceptions |= tree->OperExceptions(m_compiler); + return WALK_CONTINUE; + } + }; + + // 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)); + return walker.GetFlags(); +} + /*****************************************************************************/ struct ComplexityStruct diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 5b88e3219f6ed1..afd71a684725b7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -130,6 +130,45 @@ 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); +} + +inline constexpr ExceptionSetFlags operator|(ExceptionSetFlags a, ExceptionSetFlags b) +{ + return (ExceptionSetFlags)((uint32_t)a | (uint32_t)b); +} + +inline constexpr ExceptionSetFlags operator&(ExceptionSetFlags a, ExceptionSetFlags b) +{ + return (ExceptionSetFlags)((uint32_t)a & (uint32_t)b); +} + +inline ExceptionSetFlags& operator|=(ExceptionSetFlags& a, ExceptionSetFlags b) +{ + return a = (ExceptionSetFlags)((uint32_t)a | (uint32_t)b); +} + +inline ExceptionSetFlags& operator&=(ExceptionSetFlags& a, ExceptionSetFlags b) +{ + return a = (ExceptionSetFlags)((uint32_t)a & (uint32_t)b); +} + #ifdef DEBUG /***************************************************************************** * @@ -1815,6 +1854,7 @@ struct GenTree bool OperRequiresCallFlag(Compiler* comp); bool OperMayThrow(Compiler* comp); + ExceptionSetFlags OperExceptions(Compiler* comp); unsigned GetScaleIndexMul(); unsigned GetScaleIndexShf(); @@ -4583,6 +4623,7 @@ class CallArgs void RemovedWellKnownArg(WellKnownArg arg); regNumber GetCustomRegister(Compiler* comp, CorInfoCallConvExtension cc, WellKnownArg arg); void SplitArg(CallArg* arg, unsigned numRegs, unsigned numSlots); + void SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs); public: CallArgs(); @@ -4627,7 +4668,6 @@ class CallArgs void AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call); void ArgsComplete(Compiler* comp, GenTreeCall* call); - void SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs); void EvalArgsToTemps(Compiler* comp, GenTreeCall* call); void SetNeedsTemp(CallArg* arg); bool IsNonStandard(Compiler* comp, GenTreeCall* call, CallArg* arg); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1758051daebfad..276d0063505881 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -735,6 +735,12 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) unsigned argCount = CountArgs(); + // Previous argument with GTF_EXCEPT + GenTree* prevExceptionTree = nullptr; + // Exceptions previous tree with GTF_EXCEPT may throw (computed lazily, may + // be empty) + ExceptionSetFlags prevExceptionFlags = ExceptionSetFlags::None; + for (CallArg& arg : Args()) { GenTree* argx = arg.GetEarlyNode(); @@ -828,6 +834,8 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } bool treatLikeCall = ((argx->gtFlags & GTF_CALL) != 0); + + ExceptionSetFlags exceptionFlags = ExceptionSetFlags::None; #if FEATURE_FIXED_OUT_ARGS // Like calls, if this argument has a tree that will do an inline throw, // a call to a jit helper, then we need to treat it like a call (but only @@ -836,32 +844,35 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) // conservative, but I want to avoid as much special-case debug-only code // as possible, so leveraging the GTF_CALL flag is the easiest. // - if (!treatLikeCall && (argx->gtFlags & GTF_EXCEPT) && (argCount > 1) && comp->opts.compDbgCode && - (comp->fgWalkTreePre(&argx, Compiler::fgChkThrowCB) == Compiler::WALK_ABORT)) + if (!treatLikeCall && (argx->gtFlags & GTF_EXCEPT) && (argCount > 1) && comp->opts.compDbgCode) { - for (CallArg& otherArg : Args()) + exceptionFlags = comp->gtCollectExceptions(argx); + if ((exceptionFlags & (ExceptionSetFlags::IndexOutOfRangeException | + ExceptionSetFlags::OverflowException)) != ExceptionSetFlags::None) { - if (&otherArg == &arg) + for (CallArg& otherArg : Args()) { - continue; - } + if (&otherArg == &arg) + { + continue; + } - if (otherArg.AbiInfo.GetRegNum() == REG_STK) - { - treatLikeCall = true; - break; + if (otherArg.AbiInfo.GetRegNum() == REG_STK) + { + treatLikeCall = true; + break; + } } } } #endif // FEATURE_FIXED_OUT_ARGS - /* If it contains a call (GTF_CALL) then itself and everything before the call - with a GLOB_EFFECT must eval to temp (this is because everything with SIDE_EFFECT - has to be kept in the right order since we will move the call to the first position) + // If it contains a call (GTF_CALL) then itself and everything before the call + // with a GLOB_EFFECT must eval to temp (this is because everything with SIDE_EFFECT + // has to be kept in the right order since we will move the call to the first position) - For calls we don't have to be quite as conservative as we are with an assignment - since the call won't be modifying any non-address taken LclVars. - */ + // For calls we don't have to be quite as conservative as we are with an assignment + // since the call won't be modifying any non-address taken LclVars. if (treatLikeCall) { @@ -917,6 +928,64 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) #endif } } + else if ((argx->gtFlags & GTF_EXCEPT) != 0) + { + // If a previous arg may throw a different exception than this arg + // then we evaluate all previous arguments with GTF_EXCEPT to temps + // to avoid reordering them in our sort later. + if (prevExceptionTree != nullptr) + { + if (prevExceptionFlags == ExceptionSetFlags::None) + { + prevExceptionFlags = comp->gtCollectExceptions(prevExceptionTree); + } + + if (exceptionFlags == ExceptionSetFlags::None) + { + exceptionFlags = comp->gtCollectExceptions(argx); + } + + bool exactlyOne = isPow2(static_cast(exceptionFlags)); + bool throwsSameAsPrev = exactlyOne && (exceptionFlags == prevExceptionFlags); + if (!throwsSameAsPrev) + { + JITDUMP("Exception set for arg [%06u] interferes with previous tree [%06u]; must evaluate previous " + "trees with exceptions to temps\n", + Compiler::dspTreeID(argx), Compiler::dspTreeID(prevExceptionTree)); + + for (CallArg& prevArg : Args()) + { + if (&prevArg == &arg) + { + break; + } + +#if !FEATURE_FIXED_OUT_ARGS + if (prevArg.AbiInfo.GetRegNum() == REG_STK) + { + // All stack args are already evaluated and placed in order + // in this case. + break; + } +#endif + // Invariant here is that all nodes that were not + // already evaluated into temps and that throw can only + // be throwing the same single exception as the + // previous tree, so all of them interfere in the same + // way with the current arg and must be evaluated + // early. + if ((prevArg.GetEarlyNode() != nullptr) && + ((prevArg.GetEarlyNode()->gtFlags & GTF_EXCEPT) != 0)) + { + SetNeedsTemp(&prevArg); + } + } + } + } + + prevExceptionTree = argx; + prevExceptionFlags = exceptionFlags; + } #if FEATURE_MULTIREG_ARGS // For RyuJIT backend we will expand a Multireg arg into a GT_FIELD_LIST @@ -1121,12 +1190,7 @@ void CallArgs::SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs) { assert(m_argsComplete); -#ifdef DEBUG - if (comp->verbose) - { - printf("\nSorting the arguments:\n"); - } -#endif + JITDUMP("\nSorting the arguments:\n"); // Shuffle the arguments around before we build the late args list. The // idea is to move all "simple" arguments like constants and local vars to @@ -1794,7 +1858,7 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call) #ifdef DEBUG if (comp->verbose) { - printf("\nShuffled argument table: "); + printf("\nRegister placement order: "); for (CallArg& arg : LateArgs()) { if (arg.AbiInfo.GetRegNum() != REG_STK) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.cs b/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.cs new file mode 100644 index 00000000000000..5749133e9ee559 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.cs @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; + +public class Runtime_63905 +{ + public static int Main() + { + C c = GetNull(); + int i = GetOne(); + try + { + Foo(c.Field, i / 0); + } + catch (NullReferenceException) + { + Console.WriteLine("PASS: Caught NullReferenceException in first argument"); + return 100; + } + catch (DivideByZeroException) + { + Console.WriteLine("FAIL: Arguments were reordered incorrectly"); + return -1; + } + + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static C GetNull() => null; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int GetOne() => 1; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Foo(object o, int val) + { + } + + private class C + { + public object Field; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.csproj new file mode 100644 index 00000000000000..f492aeac9d056b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_63905/Runtime_63905.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file