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

Improve updating classes for generics #88277

Closed
wants to merge 2 commits into from

Conversation

MichalPetryka
Copy link
Contributor

@MichalPetryka MichalPetryka commented Jun 30, 2023

Informs the JIT that object or IEnumerable (and other non generic types) aren't better than List<_Canon>.

Split off from #88163.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 30, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 30, 2023
@ghost
Copy link

ghost commented Jun 30, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Informs the jit that object and IEnumerable aren't better than List<_Canon>.

Author: MichalPetryka
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@MichalPetryka
Copy link
Contributor Author

@MihuBot

@EgorBo
Copy link
Member

EgorBo commented Jul 1, 2023

No diffs so apparently it's not needed?

@MichalPetryka
Copy link
Contributor Author

No diffs so apparently it's not needed?

If you check the diffs instead of the summary, you can see that there are a bunch more variables marked ax exact type. Could the lack of assembly diffs be explained with the fact that inlining of shared generics is currently limited?
Additionally, the previous logic was pretty much flawed, blocking things like updating exactness with same type, blocking updating shared generic with exact, non-shared type and saying that non-shared base types were better than the shared generic.

@EgorBo
Copy link
Member

EgorBo commented Jul 1, 2023

No diffs so apparently it's not needed?

If you check the diffs instead of the summary, you can see that there are a bunch more variables marked ax exact type. Could the lack of assembly diffs be explained with the fact that inlining of shared generics is currently limited? Additionally, the previous logic was pretty much flawed, blocking things like updating exactness with same type, blocking updating shared generic with exact, non-shared type and saying that non-shared base types were better than the shared generic.

Could the lack of assembly diffs be explained with the fact that inlining of shared generics is currently limited?

I assume it's possible, but then we'd better experiment when we have that sort of inlining because as of now it's not clear what this change brings. Although, we do support inlining of shared generics in some cases (when they don't need a runtime lookup in fact or when generic methods are in the same Class<T>)

@MichalPetryka
Copy link
Contributor Author

@MihuBot

@MichalPetryka
Copy link
Contributor Author

@EgorBo After the fixes to the jit-diff bot, the PR now has some nice diffs (they mostly aren't reflected in size though since most of them seem to be replacing interface calls with direct ones).

@EgorBo
Copy link
Member

EgorBo commented Jul 5, 2023

@EgorBo After the fixes to the jit-diff bot, the PR now has some nice diffs (they mostly aren't reflected in size though since most of them seem to be replacing interface calls with direct ones).

can you show some examples? it is hard to believe an interface call has the same codegen size as a direct one

@MichalPetryka
Copy link
Contributor Author

can you show some examples? it is hard to believe an interface call has the same codegen size as a direct one

@@ -224662,9 +224662,9 @@ G_M64338_IG04:
 						;; size=9 bbWeight=0.50 PerfScore 1.75
 G_M64338_IG05:
        mov      rdi, r14
-       mov      rdx, r15
        mov      rsi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Collections.IStructuralEquatable:Equals(System.Object,System.Collections.IEqualityComparer):bool:this
+       mov      rdx, r15
+       mov      rax, 0xD1FFAB1E      ; code for System.Array:System.Collections.IStructuralEquatable.Equals(System.Object,System.Collections.IEqualityComparer):bool:this
        cmp      dword ptr [rdi], edi
 						;; size=21 bbWeight=0.50 PerfScore 2.00
 G_M64338_IG06:
@@ -224673,7 +224673,7 @@ G_M64338_IG06:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Collections.IStructuralEquatable:Equals(System.Object,System.Collections.IEqualityComparer):bool:this
+       tail.jmp [rax]System.Array:System.Collections.IStructuralEquatable.Equals(System.Object,System.Collections.IEqualityComparer):bool:this
 						;; size=11 bbWeight=0.50 PerfScore 2.25
 
 ; Total bytes of code 158, prolog size 19, PerfScore 40.80, instruction count 48, allocated bytes for code 158 (MethodHash=5d2c04ad) for method System.Collections.Immutable.ImmutableArray`1[System.__Canon]:System.Collections.IStructuralEquatable.Equals(System.Object,System.Collections.IEqualityComparer):bool:this (FullOpts)
@@ -224759,17 +224759,17 @@ G_M35883_IG07:
 						;; size=6 bbWeight=0.50 PerfScore 0.88
 G_M35883_IG08:
        mov      rsi, rax
-       mov      r11, 0xD1FFAB1E      ; code for System.Collections.IStructuralEquatable:GetHashCode(System.Collections.IEqualityComparer):int:this
-       call     [r11]System.Collections.IStructuralEquatable:GetHashCode(System.Collections.IEqualityComparer):int:this
+       mov      rax, 0xD1FFAB1E      ; code for System.Array:System.Collections.IStructuralEquatable.GetHashCode(System.Collections.IEqualityComparer):int:this
+       call     [rax]System.Array:System.Collections.IStructuralEquatable.GetHashCode(System.Collections.IEqualityComparer):int:this
        nop      
-						;; size=17 bbWeight=0.50 PerfScore 1.88
+						;; size=16 bbWeight=0.50 PerfScore 1.88
 G_M35883_IG09:
        add      rsp, 16
        pop      rbp
        ret      
 						;; size=6 bbWeight=0.50 PerfScore 0.88
 
-; Total bytes of code 114, prolog size 20, PerfScore 30.98, instruction count 34, allocated bytes for code 114 (MethodHash=2def73d4) for method System.Collections.Immutable.ImmutableArray`1[System.__Canon]:System.Collections.IStructuralEquatable.GetHashCode(System.Collections.IEqualityComparer):int:this (FullOpts)
+; Total bytes of code 113, prolog size 20, PerfScore 30.88, instruction count 34, allocated bytes for code 113 (MethodHash=2def73d4) for method System.Collections.Immutable.ImmutableArray`1[System.__Canon]:System.Collections.IStructuralEquatable.GetHashCode(System.Collections.IEqualityComparer):int:this (FullOpts)
 ; ============================================================
@@ -224863,21 +224863,21 @@ G_M31026_IG05:
        xor      edi, edi
        test     r14, r14
        sete     dil
-       xor      edx, edx
+       xor      esi, esi
        test     r13, r13
-       sete     dl
-       xor      edi, edx
+       sete     sil
+       xor      edi, esi
        jne      G_M31026_IG09
-						;; size=25 bbWeight=0.50 PerfScore 2.12
+						;; size=26 bbWeight=0.50 PerfScore 2.12
 G_M31026_IG06:
        test     r13, r13
        je       SHORT G_M31026_IG08
        mov      rdi, r14
        test     rdi, rdi
        je       G_M31026_IG10
-       mov      rdx, r15
        mov      rsi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Collections.IStructuralComparable:CompareTo(System.Object,System.Collections.IComparer):int:this
+       mov      rdx, r15
+       mov      rax, 0xD1FFAB1E      ; code for System.Array:System.Collections.IStructuralComparable.CompareTo(System.Object,System.Collections.IComparer):int:this
 						;; size=33 bbWeight=0.50 PerfScore 1.75
 G_M31026_IG07:
        pop      rbx
@@ -224885,7 +224885,7 @@ G_M31026_IG07:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Collections.IStructuralComparable:CompareTo(System.Object,System.Collections.IComparer):int:this
+       tail.jmp [rax]System.Array:System.Collections.IStructuralComparable.CompareTo(System.Object,System.Collections.IComparer):int:this
 						;; size=11 bbWeight=0.50 PerfScore 2.25
 G_M31026_IG08:
        mov      rdi, 0xD1FFAB1E      ; System.ArgumentException
@@ -224943,7 +224943,7 @@ G_M31026_IG10:
        int3     
 						;; size=83 bbWeight=0 PerfScore 0.00
 
-; Total bytes of code 439, prolog size 19, PerfScore 70.28, instruction count 107, allocated bytes for code 439 (MethodHash=f38586cd) for method System.Collections.Immutable.ImmutableArray`1[System.__Canon]:System.Collections.IStructuralComparable.CompareTo(System.Object,System.Collections.IComparer):int:this (FullOpts)
+; Total bytes of code 440, prolog size 19, PerfScore 70.38, instruction count 107, allocated bytes for code 440 (MethodHash=f38586cd) for method System.Collections.Immutable.ImmutableArray`1[System.__Canon]:System.Collections.IStructuralComparable.CompareTo(System.Object,System.Collections.IComparer):int:this (FullOpts)
 ; ============================================================

@MichalPetryka
Copy link
Contributor Author

MichalPetryka commented Jul 5, 2023

There are also some inlines that happen to be exactly the same size as before too (and more interface devirts):

@@ -234855,10 +234855,10 @@ G_M65253_IG09:
 						;; size=6 bbWeight=0.50 PerfScore 2.00
 G_M65253_IG10:
        mov      rdi, r15
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.Internal.IDebuggerDisplay:get_Content():System.Object:this
-       call     [r11]System.Threading.Tasks.Dataflow.Internal.IDebuggerDisplay:get_Content():System.Object:this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.BatchBlock`1[System.__Canon]:get_DebuggerDisplayContent():System.Object:this
+       call     [rax]System.Threading.Tasks.Dataflow.BatchBlock`1[System.__Canon]:get_DebuggerDisplayContent():System.Object:this
        mov      rdx, rax
-						;; size=19 bbWeight=0.50 PerfScore 1.88
+						;; size=18 bbWeight=0.50 PerfScore 1.88
 G_M65253_IG11:
        mov      rdi, r14
        mov      rsi, 0xD1FFAB1E      ; System.Runtime.CompilerServices.DefaultInterpolatedStringHandler:AppendFormatted[System.Object](System.Object):this
@@ -234911,7 +234911,7 @@ G_M65253_IG16:
        int3     
 						;; size=13 bbWeight=0 PerfScore 0.00
 
-; Total bytes of code 382, prolog size 36, PerfScore 108.16, instruction count 101, allocated bytes for code 382 (MethodHash=8f39011a) for method System.Threading.Tasks.Dataflow.BatchBlock`1+BatchBlockTargetCore[System.__Canon]:get_DebuggerDisplayContent():System.Object:this (FullOpts)
+; Total bytes of code 381, prolog size 36, PerfScore 108.06, instruction count 101, allocated bytes for code 381 (MethodHash=8f39011a) for method System.Threading.Tasks.Dataflow.BatchBlock`1+BatchBlockTargetCore[System.__Canon]:get_DebuggerDisplayContent():System.Object:this (FullOpts)
 ; ============================================================
 
 Unwind Info:
@@ -280039,7 +280039,7 @@ G_M61916_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.BatchBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M61916_IG08:
@@ -280049,7 +280049,7 @@ G_M61916_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.BatchBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M61916_IG09:
        mov      rdi, rsi
@@ -302525,7 +302525,7 @@ G_M6956_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.BatchedJoinBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M6956_IG08:
@@ -302535,7 +302535,7 @@ G_M6956_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.BatchedJoinBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M6956_IG09:
        mov      rdi, rsi
@@ -384098,7 +384098,7 @@ G_M65183_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.BroadcastBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M65183_IG08:
@@ -384108,7 +384108,7 @@ G_M65183_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.BroadcastBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M65183_IG09:
        mov      rdi, rsi
@@ -417014,7 +417014,7 @@ G_M9986_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.BufferBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M9986_IG08:
@@ -417024,7 +417024,7 @@ G_M9986_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.BufferBlock`1[System.__Canon]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M9986_IG09:
        mov      rdi, rsi
@@ -439687,7 +439687,7 @@ G_M7316_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.JoinBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M7316_IG08:
@@ -439697,7 +439697,7 @@ G_M7316_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.JoinBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M7316_IG09:
        mov      rdi, rsi
@@ -472255,7 +472255,7 @@ G_M32213_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.TransformBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M32213_IG08:
@@ -472265,7 +472265,7 @@ G_M32213_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.TransformBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M32213_IG09:
        mov      rdi, rsi
@@ -521748,7 +521748,7 @@ G_M30752_IG07:
        call     [rax]System.Threading.Tasks.Task:get_Exception():System.AggregateException:this
        mov      rsi, rax
        mov      rdi, r13
-       mov      r11, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       mov      rax, 0xD1FFAB1E      ; code for System.Threading.Tasks.Dataflow.TransformManyBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
        cmp      dword ptr [rdi], edi
 						;; size=35 bbWeight=1 PerfScore 10.25
 G_M30752_IG08:
@@ -521758,7 +521758,7 @@ G_M30752_IG08:
        pop      r14
        pop      r15
        pop      rbp
-       tail.jmp [r11]System.Threading.Tasks.Dataflow.IDataflowBlock:Fault(System.Exception):this
+       tail.jmp [rax]System.Threading.Tasks.Dataflow.TransformManyBlock`2[System.__Canon,System.Nullable`1[int]]:System.Threading.Tasks.Dataflow.IDataflowBlock.Fault(System.Exception):this
 						;; size=15 bbWeight=1 PerfScore 4.75
 G_M30752_IG09:
        mov      rdi, rsi

@MichalPetryka
Copy link
Contributor Author

@MihuBot

@MichalPetryka
Copy link
Contributor Author

@MihuBot

@JulieLeeMSFT
Copy link
Member

@MihuBot, this is a reminder to review this community PR.

@MihaZupan
Copy link
Member

MihaZupan commented Aug 28, 2023

@JulieLeeMSFT MihuBot is just an automated tool to build PRs and collect jit-diff results, the mentions are a way to trigger it :)

@JulieLeeMSFT
Copy link
Member

Then we will wait for a review from @EgorBo.

@EgorBo
Copy link
Member

EgorBo commented Aug 28, 2023

Then we will wait for a review from @EgorBo.

I think we already discussed this offile that we don't see a clear value (full diffs?) here and potential risks since it touches very sensitive things

@MichalPetryka
Copy link
Contributor Author

I think we already discussed this offile that we don't see a clear value (full diffs?) here and potential risks since it touches very sensitive things

IIRC the final decision was to wait with this for shared generic inlining (which you were planning to improve in .Net 9).

@JulieLeeMSFT JulieLeeMSFT marked this pull request as draft September 25, 2023 20:06
@JulieLeeMSFT
Copy link
Member

Converted to draft until it is further ready with clearer value after the shared generic inlining is done.

@ghost ghost closed this Oct 25, 2023
@ghost
Copy link

ghost commented Oct 25, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2023
This pull request was closed.
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants