Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Dec 17, 2020

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 29, 2020

Closes #45302

Example:

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

master:

G_M38234_IG01:
       sub      rsp, 40
G_M38234_IG02:
       cmp      dword ptr [rdx], edx
       mov      rcx, rdx
       call     System.Object:MemberwiseClone():System.Object:this
       mov      rdx, rax
       mov      rcx, 0xD1FFAB1E
       call     CORINFO_HELP_CHKCASTARRAY
       nop      
G_M38234_IG03:
       add      rsp, 40
       ret

PR:

G_M38234_IG01:
       sub      rsp, 40
G_M38234_IG02:
       cmp      dword ptr [rdx], edx
       mov      rcx, rdx
       call     System.Object:MemberwiseClone():System.Object:this
       nop      
G_M38234_IG03:
       add      rsp, 40
       ret

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 --pmi
Beginning 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.59s
Diffs (if any) can be viewed by comparing: C:\prj\jit-diffs\dasmset_5\base C:\prj\jit-diffs\dasmset_5\diff

git diff --no-index --diff-filter=M --exit-code --numstat C:\prj\jit-diffs\dasmset_5\diff C:\prj\jit-diffs\dasmset_5\base

Analyzing CodeSize diffs...
Found 36 files with textual diffs.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies [invoking .cctors] for  default jit

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 49740193
Total bytes of diff: 49736462
Total 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():this

Top 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:this

120 total methods with Code Size differences (120 improved, 0 regressed), 258751 unchanged.
Completed analysis in 29.25s

/cc @GrabYourPitchforks @VSadov

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 29, 2020
@EgorBo
Copy link
Member Author

EgorBo commented Nov 29, 2020

Failures are not related (#45317)

@VSadov
Copy link
Member

VSadov commented Nov 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)

Copy link
Member

@VSadov 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

@ViktorHofer
Copy link
Member

// 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
Copy link
Member Author

EgorBo commented Dec 15, 2020

@dotnet/jit-contrib can this be merged?

@AndyAyersMS
Copy link
Member

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

@AndyAyersMS
Copy link
Member

Are you going to re-run diffs?

@EgorBo
Copy link
Member Author

EgorBo commented Dec 15, 2020

Perhaps teach gtGetClassHandle 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.

Copy link
Member

@AndyAyersMS 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 Outdated Show resolved Hide resolved
Co-authored-by: Andy Ayers <andya@microsoft.com>
@AndyAyersMS
Copy link
Member

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.

@AndyAyersMS
Copy link
Member

Thanks Egor!

@AndyAyersMS AndyAyersMS merged commit acd4855 into dotnet:master Dec 17, 2020
@GrabYourPitchforks
Copy link
Member

A random question about this line:

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

I assume it's ok that objClass 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 call clone2.GetType(), the JIT won't try to over-optimize this, and it'll correctly return typeof(uint[]) instead of typeof(int[])?

@pentp
Copy link
Contributor

pentp commented Dec 18, 2020

Could the same logic be applied to MemberwiseClone also?

@AndyAyersMS
Copy link
Member

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:

//------------------------------------------------------------------------
// 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.
bool Compiler::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)
{
return true;
}
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))
{
return impIsClassExact(arrayElementHandle);
}
}
return false;
}

In the above we would not know the exact types for arr1 and arr2 and so partial knowledge of those types would not allow optimizing a subsequent GetType().

1 similar comment
@AndyAyersMS

This comment has been minimized.

@AndyAyersMS
Copy link
Member

Could the same logic be applied to MemberwiseClone also?

Seems like it could, yes. @EgorBo want to follow up on this?

@ghost ghost locked as resolved and limited conversation to collaborators Jan 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT should optimize away the cast that occurs after Array.Clone
7 participants