NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

RyuJIT: Don't emit cast helpers for (T)array.Clone()#45311

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

Jump to bottom

Merged

AndyAyersMS merged 5 commits intodotnet:masterfromEgorBo:jit-array-clone2

Dec 17, 2020

Merged

RyuJIT: Don't emit cast helpers for (T)array.Clone()#45311

AndyAyersMS merged 5 commits intodotnet:masterfromEgorBo:jit-array-clone2

Dec 17, 2020

Conversation

Copy link

Member

EgorBo commentedNov 29, 2020

Closes#45302

Example:

int[]CloneArray(int[]a)=>(int[])a.Clone();

master:

G_M38234_IG01:subrsp,40G_M38234_IG02:cmp      dword ptr[rdx],edxmovrcx,rdxcall     System.Object:MemberwiseClone():System.Object:thismovrdx,raxmovrcx,0xD1FFAB1Ecall     CORINFO_HELP_CHKCASTARRAYnopG_M38234_IG03:addrsp,40ret

PR:

G_M38234_IG01:subrsp,40G_M38234_IG02:cmp      dword ptr[rdx],edxmovrcx,rdxcall     System.Object:MemberwiseClone():System.Object:thisnopG_M38234_IG03:addrsp,40ret

Jit-diff (--pmi):

C:\prj>jit-diff diff --output C:\prj\jit-diffs --cctors -f --core_root runtime\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root --base C:\prj\runtime\artifacts\bin\coreclr\windows.x64.Checked_base --diff C:\prj\runtime\artifacts\bin\coreclr\windows.x64.Checked --pmiBeginning PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies/ Finished 268/268 Base 268/268 Diff [508.4 sec]Completed PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies [invoking .cctors] in 508.59sDiffs (if any) can be viewed by comparing: C:\prj\jit-diffs\dasmset_5\base C:\prj\jit-diffs\dasmset_5\diffgit diff --no-index --diff-filter=M --exit-code --numstat C:\prj\jit-diffs\dasmset_5\diff C:\prj\jit-diffs\dasmset_5\baseAnalyzing CodeSize diffs...Found 36 files with textual diffs.PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies [invoking .cctors] for  default jitSummary of Code Size diffs:(Lower is better)Total bytes of base: 49740193Total bytes of diff: 49736462Total bytes of delta: -3731 (-0.01% of base)    diff is an improvement.Top file improvements (bytes):        -947 : System.Private.CoreLib.dasm (-0.02% of base)        -576 : FSharp.Core.dasm (-0.02% of base)        -378 : System.Numerics.Tensors.dasm (-0.15% of base)        -288 : System.Security.Cryptography.Algorithms.dasm (-0.09% of base)        -224 : Microsoft.CodeAnalysis.dasm (-0.01% of base)        -108 : System.Collections.Immutable.dasm (-0.01% of base)        -108 : System.ComponentModel.TypeConverter.dasm (-0.04% of base)        -108 : System.Threading.Tasks.Dataflow.dasm (-0.01% of base)         -90 : System.Security.Cryptography.Primitives.dasm (-0.25% of base)         -90 : System.Web.HttpUtility.dasm (-0.83% of base)         -72 : Microsoft.VisualBasic.Core.dasm (-0.02% of base)         -72 : System.Private.Xml.dasm (-0.00% of base)         -72 : System.Runtime.Numerics.dasm (-0.10% of base)         -54 : System.Security.Claims.dasm (-0.24% of base)         -54 : System.Security.Cryptography.Csp.dasm (-0.10% of base)         -54 : System.Security.Cryptography.Xml.dasm (-0.03% of base)         -40 : System.Reflection.Metadata.dasm (-0.01% of base)         -36 : System.CodeDom.dasm (-0.02% of base)         -36 : System.DirectoryServices.AccountManagement.dasm (-0.01% of base)         -36 : System.Reflection.MetadataLoadContext.dasm (-0.02% of base)36 total files with Code Size differences (36 improved, 0 regressed), 232 unchanged.Top method improvements (bytes):        -234 (-12.09% of base) : System.Private.CoreLib.dasm - HashSet`1:ConstructFrom(HashSet`1):this (7 methods)        -126 (-3.37% of base) : FSharp.Core.dasm - Array:stableSortWithKeysAndComparer(IComparer`1,IComparer`1,ref,ref) (7 methods)        -126 (-5.53% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:.ctor(ReadOnlySpan`1,int,bool):this (7 methods)        -126 (-9.09% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:.ctor(Memory`1,Memory`1,Memory`1,int,ReadOnlySpan`1,bool):this (7 methods)        -126 (-1.53% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:.ctor(Array,bool):this (7 methods)        -108 (-14.84% of base) : FSharp.Core.dasm - ArrayModule:Copy(ref):ref (7 methods)        -108 (-4.27% of base) : FSharp.Core.dasm - SeqModule:ToArray(IEnumerable`1):ref (7 methods)        -108 (-8.34% of base) : FSharp.Core.dasm - Array:stableSortInPlace(ref) (7 methods)        -108 (-10.41% of base) : FSharp.Core.dasm - Array:stableSortInPlaceWith(FSharpFunc`2,ref) (7 methods)        -108 (-17.20% of base) : System.Collections.Immutable.dasm - ImmutableArrayExtensions:ToArray(ImmutableArray`1):ref (7 methods)        -108 (-21.34% of base) : System.Threading.Tasks.Dataflow.dasm - ImmutableArray`1:ToArray():ref:this (7 methods)        -103 (-12.50% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:LeftComplex(AvlNode):AvlNode (8 base, 7 diff methods)        -103 (-12.50% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:RightComplex(AvlNode):AvlNode (8 base, 7 diff methods)         -54 (-8.44% of base) : System.ComponentModel.TypeConverter.dasm - PropertyTabAttribute:InitializeArrays(ref,ref,ref):this         -54 (-0.52% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this         -40 (-23.26% of base) : System.Reflection.Metadata.dasm - BlobHeap:GetVirtualBlobBytes(BlobHandle,bool):ref:this         -36 (-13.53% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:GetMergedPatterns(ref,String):ref         -36 (-23.08% of base) : System.Security.Cryptography.Algorithms.dasm - Aes:.ctor():this         -36 (-24.32% of base) : System.Security.Cryptography.Algorithms.dasm - DES:.ctor():this         -36 (-24.32% of base) : System.Security.Cryptography.Algorithms.dasm - RC2:.ctor():thisTop method improvements (percentages):         -18 (-51.43% of base) : System.Security.Cryptography.Algorithms.dasm - KeySizeHelpers:CloneKeySizesArray(ref):ref         -18 (-51.43% of base) : System.Security.Cryptography.Csp.dasm - KeySizeHelpers:CloneKeySizesArray(ref):ref         -18 (-46.15% of base) : FSharp.Core.dasm - CompilationArgumentCountsAttribute:get_Counts():IEnumerable`1:this         -18 (-46.15% of base) : Microsoft.CSharp.dasm - ComTypeEnumDesc:GetMemberNames():ref:this         -18 (-46.15% of base) : System.CodeDom.dasm - CompilerInfo:GetLanguages():ref:this         -18 (-46.15% of base) : System.CodeDom.dasm - CompilerInfo:GetExtensions():ref:this         -18 (-46.15% of base) : System.Net.NetworkInformation.dasm - PhysicalAddress:GetAddressBytes():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - ApplicationId:get_PublicKeyToken():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - NumberFormatInfo:get_CurrencyGroupSizes():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - NumberFormatInfo:get_NumberGroupSizes():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - NumberFormatInfo:get_PercentGroupSizes():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - SortKey:get_KeyData():ref:this         -18 (-46.15% of base) : System.Private.CoreLib.dasm - SignatureConstructedGenericType:get_GenericTypeArguments():ref:this         -18 (-46.15% of base) : System.Security.Cryptography.Algorithms.dasm - ECDiffieHellmanPublicKey:ToByteArray():ref:this         -18 (-46.15% of base) : System.Security.Cryptography.Primitives.dasm - AsymmetricAlgorithm:get_LegalKeySizes():ref:this         -18 (-46.15% of base) : System.Security.Cryptography.Primitives.dasm - SymmetricAlgorithm:get_LegalBlockSizes():ref:this         -18 (-46.15% of base) : System.Security.Cryptography.Primitives.dasm - SymmetricAlgorithm:get_LegalKeySizes():ref:this         -18 (-42.86% of base) : System.Private.CoreLib.dasm - NumberFormatInfo:get_NativeDigits():ref:this         -18 (-41.86% of base) : System.Reflection.Context.dasm - VirtualPropertyBase:GetIndexParameters():ref:this         -18 (-40.00% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:get_MonthGenitiveNames():ref:this120 total methods with Code Size differences (120 improved, 0 regressed), 258751 unchanged.Completed analysis in 29.25s

/cc@GrabYourPitchforks @VSadov

Dotnet-GitSync-Bot added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label

Nov 29, 2020

Copy link

MemberAuthor

EgorBo commentedNov 29, 2020

Failures are not related (#45317)

Copy link

Member

VSadov commentedNov 30, 2020

// op1 - value to cast

Nit: should rename to "tree" now?

Refers to: src/coreclr/src/jit/importer.cpp:10719 in ce5ac35. [](commit_id = ce5ac357154f11e14ad6d4889ae215e49456b44f, deletion_comment = False)

VSadov reviewed

Nov 30, 2020

View reviewed changes

src/coreclr/src/jit/importer.cpp OutdatedShow resolvedHide resolved

VSadov approved these changes

Nov 30, 2020

View reviewed changes

Copy link

Member

VSadov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM

Copy link

Member

ViktorHofer commentedDec 8, 2020

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script:https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

EgorBo force-pushed thejit-array-clone2 branch fromc2e0dde to169ec92Compare

December 10, 2020 16:48

EgorBo added2 commits

December 11, 2020 17:24

Drop cast helper call for (To)array.Clone()

d7826ee

Address feedback

723da1e

EgorBo force-pushed thejit-array-clone2 branch from169ec92 to723da1eCompare

December 11, 2020 14:25

Copy link

MemberAuthor

EgorBo commentedDec 15, 2020

@dotnet/jit-contrib can this be merged?

Copy link

Member

AndyAyersMS commentedDec 15, 2020

Perhaps teachgtGetClassHandle about this intrinsic instead? That might convey some extra benefits...

EgorBo added2 commits

December 15, 2020 21:46

Address feedback

4405560

Merge branch 'master' of github.com:dotnet/runtime into jit-array-clone2

fc82c3a

Copy link

Member

AndyAyersMS commentedDec 15, 2020

Are you going to re-run diffs?

Copy link

MemberAuthor

EgorBo commentedDec 15, 2020

Perhaps teachgtGetClassHandle about this intrinsic instead? That might convey some extra benefits...

Good point, it looks nicer now

Are you going to re-run diffs?

Yeah just finished - the diff is exactly the same.

AndyAyersMS approved these changes

Dec 15, 2020

View reviewed changes

Copy link

Member

AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good -- just one small comment.

src/coreclr/jit/gentree.cpp OutdatedShow resolvedHide resolved

Update src/coreclr/jit/gentree.cpp

bd6b07f

Co-authored-by: Andy Ayers <andya@microsoft.com>

Copy link

Member

AndyAyersMS commentedDec 17, 2020

Testing hit another case of the xunit assertion#11063.

Unhandled exception. System.InvalidOperationException: Collection was modified after the enumerator was instantiated.   at System.Collections.Generic.Stack`1.Enumerator.MoveNext()   at Xunit.Sdk.DisposalTracker.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\DisposalTracker.cs:line 26

am going to rerun these.

Copy link

Member

AndyAyersMS commentedDec 17, 2020

Thanks Egor!

AndyAyersMS merged commitacd4855 intodotnet:master

Dec 17, 2020

Copy link

Member

GrabYourPitchforks commentedDec 17, 2020

A random question about this line:

objClass = gtGetClassHandle(call->gtCallThisArg->GetNode(), pIsExact, pIsNonNull);

I assume it's ok thatobjClass might technically be a lie? For example...

object[]arr1=GetArray();// actually a string[]!object[]clone1=(object[])arr1.Clone();// also actually a string[]!int[]arr2=GetArray();// actually a uint[]!int[]clone2=(int[])arr2.Clone();// also actually a uint[]!// etc.

If I then callclone2.GetType(), the JIT won't try to over-optimize this, and it'll correctly returntypeof(uint[]) instead oftypeof(int[])?

Copy link

Contributor

pentp commentedDec 18, 2020

Could the same logic be applied toMemberwiseClone also?

Copy link

Member

AndyAyersMS commentedDec 18, 2020

I assume it's ok that objClass might technically be a lie?

We're fairly cautious when reasoning about types unless we know the exact type (say from seeing a constructor call or an exact type test). And even when we do know the exact type we're cautious with arrays of primitive types:

runtime/src/coreclr/jit/importer.cpp

Lines 21629 to 21663 infcf0b8b

	//------------------------------------------------------------------------
	// impIsClassExact: check if a class handle can only describe values
	// of exactly one class.
	//
	// Arguments:
	// classHnd - handle for class in question
	//
	// Returns:
	// true if class is final and not subject to special casting from
	// variance or similar.
	//
	// Note:
	// We are conservative on arrays of primitive types here.

	boolCompiler::impIsClassExact(CORINFO_CLASS_HANDLE classHnd)
	{
	DWORD flags = info.compCompHnd->getClassAttribs(classHnd);
	DWORD flagsMask = CORINFO_FLG_FINAL \| CORINFO_FLG_VARIANCE \| CORINFO_FLG_ARRAY;

	if ((flags & flagsMask) == CORINFO_FLG_FINAL)
	{
	returntrue;
	}
	if ((flags & flagsMask) == (CORINFO_FLG_FINAL \| CORINFO_FLG_ARRAY))
	{
	CORINFO_CLASS_HANDLE arrayElementHandle =nullptr;
	CorInfoType type = info.compCompHnd->getChildType(classHnd, &arrayElementHandle);

	if ((type == CORINFO_TYPE_CLASS) \|\| (type == CORINFO_TYPE_VALUECLASS))
	{
	returnimpIsClassExact(arrayElementHandle);
	}
	}
	returnfalse;
	}

In the above we would not know the exact types forarr1 andarr2 and so partial knowledge of those types would not allow optimizing a subsequentGetType().