From 51306837c5f43cd1a34084d1b42ae9d4f812d82f Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 15 Jul 2024 16:38:09 +0200 Subject: [PATCH] JIT: Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0 (#104815) Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/gentree.h | 8 +- src/coreclr/jit/importercalls.cpp | 109 ++++++++++++++++++ src/coreclr/jit/namedintrinsiclist.h | 2 + .../src/System/ArgumentNullException.cs | 2 + 5 files changed, 122 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index efd73852b1a1c..2f9abcb00703a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4541,6 +4541,8 @@ class Compiler GenTree* impDuplicateWithProfiledArg(GenTreeCall* call, IL_OFFSET ilOffset); + GenTree* impThrowIfNull(GenTreeCall* call); + #ifdef DEBUG var_types impImportJitTestLabelMark(int numArgs); #endif // DEBUG diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 5b7bc8278cab7..c2b186e691e89 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2265,6 +2265,7 @@ struct GenTree return OperGet() == GT_CALL; } inline bool IsHelperCall(); + inline bool IsHelperCall(Compiler* compiler, unsigned helper); bool gtOverflow() const; bool gtOverflowEx() const; @@ -10500,7 +10501,12 @@ inline bool GenTree::IsCnsVec() const inline bool GenTree::IsHelperCall() { - return OperGet() == GT_CALL && AsCall()->IsHelperCall(); + return IsCall() && AsCall()->IsHelperCall(); +} + +inline bool GenTree::IsHelperCall(Compiler* compiler, unsigned helper) +{ + return IsCall() && AsCall()->IsHelperCall(compiler, helper); } inline var_types GenTree::CastFromType() diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index dc205c60c2fd0..0d2045d553c82 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1330,6 +1330,10 @@ var_types Compiler::impImportCall(OPCODE opcode, } else { + if (call->IsCall() && call->AsCall()->IsSpecialIntrinsic(this, NI_System_ArgumentNullException_ThrowIfNull)) + { + call = impThrowIfNull(call->AsCall()); + } impAppendTree(call, CHECK_SPILL_ALL, impCurStmtDI); } } @@ -1528,6 +1532,97 @@ var_types Compiler::impImportCall(OPCODE opcode, #pragma warning(pop) #endif +//------------------------------------------------------------------------ +// impThrowIfNull: Remove redundandant boxing from ArgumentNullException_ThrowIfNull +// it is done for Tier0 where we can't remove it without inlining otherwise. +// +// Arguments: +// call -- call representing ArgumentNullException_ThrowIfNull +// +// Return Value: +// Optimized tree (or the original call tree if we can't optimize it). +// +GenTree* Compiler::impThrowIfNull(GenTreeCall* call) +{ + // We have two overloads: + // + // void ThrowIfNull(object argument, string paramName = null) + // void ThrowIfNull(object argument, ExceptionArgument paramName) + // + assert(call->IsSpecialIntrinsic(this, NI_System_ArgumentNullException_ThrowIfNull)); + assert(call->gtArgs.CountUserArgs() == 2); + assert(call->TypeIs(TYP_VOID)); + + if (opts.compDbgCode || opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT)) + { + // Don't fold it for debug code or forced MinOpts + return call; + } + + GenTree* value = call->gtArgs.GetUserArgByIndex(0)->GetNode(); + GenTree* valueName = call->gtArgs.GetUserArgByIndex(1)->GetNode(); + + // Case 1: value-type (non-nullable): + // + // ArgumentNullException_ThrowIfNull(GT_BOX(value), valueName) + // -> + // NOP (with side-effects if any) + // + if (value->OperIs(GT_BOX)) + { + // Now we need to spill the addr and argName arguments in the correct order + // to preserve possible side effects. + unsigned boxedValTmp = lvaGrabTemp(true DEBUGARG("boxedVal spilled")); + unsigned boxedArgNameTmp = lvaGrabTemp(true DEBUGARG("boxedArg spilled")); + impStoreToTemp(boxedValTmp, value, CHECK_SPILL_ALL); + impStoreToTemp(boxedArgNameTmp, valueName, CHECK_SPILL_ALL); + gtTryRemoveBoxUpstreamEffects(value, BR_REMOVE_AND_NARROW); + return gtNewNothingNode(); + } + + // Case 2: nullable: + // + // ArgumentNullException.ThrowIfNull(CORINFO_HELP_BOX_NULLABLE(classHandle, addr), valueName); + // -> + // addr->hasValue != 0 ? NOP : ArgumentNullException.ThrowIfNull(null, valueNameTmp) + // + if (opts.OptimizationEnabled() || !value->IsHelperCall(this, CORINFO_HELP_BOX_NULLABLE)) + { + // We're not boxing - bail out. + // NOTE: when opts are enabled, we remove the box as is (with better CQ) + return call; + } + + GenTree* boxHelperClsArg = value->AsCall()->gtArgs.GetUserArgByIndex(0)->GetNode(); + GenTree* boxHelperAddrArg = value->AsCall()->gtArgs.GetUserArgByIndex(1)->GetNode(); + + if ((boxHelperClsArg->gtFlags & GTF_SIDE_EFFECT) != 0) + { + // boxHelperClsArg is always just a class handle constant, so we don't bother spilling it. + return call; + } + + // Now we need to spill the addr and argName arguments in the correct order + // to preserve possible side effects. + unsigned boxedValTmp = lvaGrabTemp(true DEBUGARG("boxedVal spilled")); + unsigned boxedArgNameTmp = lvaGrabTemp(true DEBUGARG("boxedArg spilled")); + impStoreToTemp(boxedValTmp, boxHelperAddrArg, CHECK_SPILL_ALL); + impStoreToTemp(boxedArgNameTmp, valueName, CHECK_SPILL_ALL); + + // Change arguments to 'ThrowIfNull(null, valueNameTmp)' + call->gtArgs.GetUserArgByIndex(0)->SetEarlyNode(gtNewNull()); + call->gtArgs.GetUserArgByIndex(1)->SetEarlyNode(gtNewLclvNode(boxedArgNameTmp, valueName->TypeGet())); + + // This is Tier0 specific, so we create a raw indir node to access Nullable.hasValue field + // which is the first field of Nullable struct and is of type 'bool'. + // + static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + GenTree* hasValueField = gtNewIndir(TYP_UBYTE, gtNewLclvNode(boxedValTmp, boxHelperAddrArg->TypeGet())); + GenTreeOp* cond = gtNewOperNode(GT_NE, TYP_INT, hasValueField, gtNewIconNode(0)); + + return gtNewQmarkNode(TYP_VOID, cond, gtNewColonNode(TYP_VOID, gtNewNothingNode(), call)); +} + //------------------------------------------------------------------------ // impDuplicateWithProfiledArg: duplicates a call with a profiled argument, e.g.: // Given `Buffer.Memmove(dst, src, len)` call, @@ -3257,6 +3352,9 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd, // to avoid some unnecessary boxing case NI_System_Enum_HasFlag: + // This one is made intrinsic specifically to avoid boxing in Tier0 + case NI_System_ArgumentNullException_ThrowIfNull: + // Most atomics are compiled to single instructions case NI_System_Threading_Interlocked_And: case NI_System_Threading_Interlocked_Or: @@ -3787,6 +3885,10 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd, break; } + case NI_System_ArgumentNullException_ThrowIfNull: + isSpecial = true; + break; + case NI_System_Enum_HasFlag: { GenTree* thisOp = impStackTop(1).val; @@ -9826,6 +9928,13 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) result = NI_System_Activator_DefaultConstructorOf; } } + else if (strcmp(className, "ArgumentNullException") == 0) + { + if (strcmp(methodName, "ThrowIfNull") == 0) + { + result = NI_System_ArgumentNullException_ThrowIfNull; + } + } else if (strcmp(className, "Array") == 0) { if (strcmp(methodName, "Clone") == 0) diff --git a/src/coreclr/jit/namedintrinsiclist.h b/src/coreclr/jit/namedintrinsiclist.h index 0ec8fd2496ba3..93e4ce7893f98 100644 --- a/src/coreclr/jit/namedintrinsiclist.h +++ b/src/coreclr/jit/namedintrinsiclist.h @@ -13,6 +13,8 @@ enum NamedIntrinsic : unsigned short { NI_Illegal = 0, + NI_System_ArgumentNullException_ThrowIfNull, + NI_System_Enum_HasFlag, NI_System_BitConverter_DoubleToInt64Bits, diff --git a/src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs b/src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs index b563c12dfa356..8094b882ef548 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs @@ -51,6 +51,7 @@ protected ArgumentNullException(SerializationInfo info, StreamingContext context /// Throws an if is null. /// The reference type argument to validate as non-null. /// The name of the parameter with which corresponds. + [Intrinsic] // Tier0 intrinsic to avoid redundant boxing in generics public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null) { if (argument is null) @@ -59,6 +60,7 @@ public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpres } } + [Intrinsic] // Tier0 intrinsic to avoid redundant boxing in generics internal static void ThrowIfNull([NotNull] object? argument, ExceptionArgument paramName) { if (argument is null)