- Notifications
You must be signed in to change notification settings - Fork5.2k
JIT: Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0#104815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
8 commits Select commitHold shift + click to select a range
6dca675 Optimize boxing for ThrowIfNull
EgorBo3d25a88 fix debug
EgorBoae84b62 Handle nullable
EgorBo6264f31 Merge branch 'main' of github.com:dotnet/runtime into fix-alloc-throw…
EgorBod446a2b clean up
EgorBoee83f6b Update src/coreclr/jit/importercalls.cpp
EgorBo371331f Merge branch 'main' of github.com:dotnet/runtime into fix-alloc-throw…
EgorBo6a7fdf7 Address feedback
EgorBoFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletionssrc/coreclr/jit/compiler.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
8 changes: 7 additions & 1 deletionsrc/coreclr/jit/gentree.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 IsCall() && AsCall()->IsHelperCall(); | ||
| } | ||
| inline bool GenTree::IsHelperCall(Compiler* compiler, unsigned helper) | ||
| { | ||
| return IsCall() && AsCall()->IsHelperCall(compiler, helper); | ||
| } | ||
jakobbotsch marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| inline var_types GenTree::CastFromType() | ||
109 changes: 109 additions & 0 deletionssrc/coreclr/jit/importercalls.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<T>.hasValue field | ||
| // which is the first field of Nullable<T> struct and is of type 'bool'. | ||
| // | ||
| static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); | ||
| GenTree* hasValueField = gtNewIndir(TYP_UBYTE, gtNewLclvNode(boxedValTmp, boxHelperAddrArg->TypeGet())); | ||
EgorBo marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| 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) | ||
2 changes: 2 additions & 0 deletionssrc/coreclr/jit/namedintrinsiclist.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletionssrc/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.