[RyuJIT] Track exact class for special intrinsics (EqulityComparer, Comparer) #48279

EgorBo · 2021-02-14T16:57:45Z

Fixes #48273 (comment) (cc @drieseng, @stephentoub)

Now jit should be able to track exact class for these special intrinsics (EqulityComparer and Comparer) for locals (single-def ones), e.g.:

public static bool Test(int x)
{
    IEqualityComparer<int> comparer = EqualityComparer<int>.Default;
    for (int i = 0; i < 100; i++)
        if (comparer.Equals(i, x))
            return true;
    return false;
}

Codegen diff: https://www.diffchecker.com/GAPNhiJs (this PR is on the right)
CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS is still there in cases of locals though.

stephentoub · 2021-02-14T17:18:27Z

Can we eliminate code duplication like:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

Lines 531 to 611 in 65b923a

    
           if (typeof(TKey).IsValueType) 
        
           { 
        
               // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic 
        
               while (true) 
        
               { 
        
                   // Should be a while loop https://github.com/dotnet/runtime/issues/9422 
        
                   // Test uint in if rather than loop condition to drop range check for following array access 
        
                   if ((uint)i >= (uint)entries.Length) 
        
                   { 
        
                       break; 
        
                   } 
        
                   if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key)) 
        
                   { 
        
                       if (behavior == InsertionBehavior.OverwriteExisting) 
        
                       { 
        
                           entries[i].value = value; 
        
                           return true; 
        
                       } 
        
                       if (behavior == InsertionBehavior.ThrowOnExisting) 
        
                       { 
        
                           ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); 
        
                       } 
        
                       return false; 
        
                   } 
        
                   i = entries[i].next; 
        
                   collisionCount++; 
        
                   if (collisionCount > (uint)entries.Length) 
        
                   { 
        
                       // The chain of entries forms a loop; which means a concurrent update has happened. 
        
                       // Break out of the loop and throw, rather than looping forever. 
        
                       ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); 
        
                   } 
        
               } 
        
           } 
        
           else 
        
           { 
        
               // Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize 
        
               // https://github.com/dotnet/runtime/issues/10050 
        
               // So cache in a local rather than get EqualityComparer per loop iteration 
        
               EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default; 
        
               while (true) 
        
               { 
        
                   // Should be a while loop https://github.com/dotnet/runtime/issues/9422 
        
                   // Test uint in if rather than loop condition to drop range check for following array access 
        
                   if ((uint)i >= (uint)entries.Length) 
        
                   { 
        
                       break; 
        
                   } 
        
                   if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key)) 
        
                   { 
        
                       if (behavior == InsertionBehavior.OverwriteExisting) 
        
                       { 
        
                           entries[i].value = value; 
        
                           return true; 
        
                       } 
        
                       if (behavior == InsertionBehavior.ThrowOnExisting) 
        
                       { 
        
                           ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); 
        
                       } 
        
                       return false; 
        
                   } 
        
                   i = entries[i].next; 
        
                   collisionCount++; 
        
                   if (collisionCount > (uint)entries.Length) 
        
                   { 
        
                       // The chain of entries forms a loop; which means a concurrent update has happened. 
        
                       // Break out of the loop and throw, rather than looping forever. 
        
                       ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); 
        
                   } 
        
               } 
        
           }

with this?

benaadams · 2021-02-14T18:26:40Z

Can we eliminate code duplication like: ...

It would need to hoist resolving the comparer in a shared generic (where it can't devirtualize) and devirtualize in-place for a non-shared generic to work there? #10050

EgorBo · 2021-02-14T22:59:15Z

Can we eliminate code duplication like:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

Lines 531 to 611 in 65b923a

if (typeof(TKey).IsValueType)

{

// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic

while (true)

{

// Should be a while loop https://github.com/dotnet/runtime/issues/9422

// Test uint in if rather than loop condition to drop range check for following array access

if ((uint)i >= (uint)entries.Length)

{

break;

}

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))

{

if (behavior == InsertionBehavior.OverwriteExisting)

{

entries[i].value = value;

return true;

}

if (behavior == InsertionBehavior.ThrowOnExisting)

{

ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key);

}

return false;

}

i = entries[i].next;

collisionCount++;

if (collisionCount > (uint)entries.Length)

{

// The chain of entries forms a loop; which means a concurrent update has happened.

// Break out of the loop and throw, rather than looping forever.

ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();

}

}

}

else

{

// Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize

// https://github.com/dotnet/runtime/issues/10050

// So cache in a local rather than get EqualityComparer per loop iteration

EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;

while (true)

{

// Should be a while loop https://github.com/dotnet/runtime/issues/9422

// Test uint in if rather than loop condition to drop range check for following array access

if ((uint)i >= (uint)entries.Length)

{

break;

}

if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key))

{

if (behavior == InsertionBehavior.OverwriteExisting)

{

entries[i].value = value;

return true;

}

if (behavior == InsertionBehavior.ThrowOnExisting)

{

ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key);

}

return false;

}

i = entries[i].next;

collisionCount++;

if (collisionCount > (uint)entries.Length)

{

// The chain of entries forms a loop; which means a concurrent update has happened.

// Break out of the loop and throw, rather than looping forever.

ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();

}

}

}

with this?

Unfortunately while my fix indeed tells JIT that a local is of an exact type (thus, it can devirtualize calls with it) - it still introduces a static initialization (call CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS) call overhead. When we invoke EqualityComparer<T>.Default.Equals (without locals) - JIT is able to omit that static init. Usually, it's not a problem since we get rid of static initialization when we re-jit methods, but it's not the case for methods with loops - those are compiled directly to tier1 and most likely stay with that initialization forever.

I'm taking a look if I can improve this behavior in my PR so we can drop that workaround.

EgorBo · 2021-02-15T11:18:58Z

Going to leave it as is for now. In order to eliminate that redundant static initialization in case of methods-with-loops more changes are required: We need to make sure get_Default field is never used than we can drop the initialization. Something like this probably (but it doesn't work as expected yet): EgorBo@11a1d4e

EgorBo · 2021-02-15T15:42:53Z

@dotnet/jit-contrib PTAL

AndyAyersMS · 2021-02-15T16:25:33Z

Can you run diffs?

AndyAyersMS · 2021-02-15T16:35:21Z

Also add the test case from #10500 and look at diffs (thought I had added it, but looks like I didn't...)

EgorBo · 2021-02-16T18:30:36Z

Can you run diffs?

Total bytes of base: 50671644
Total bytes of diff: 50672193
Total bytes of delta: 549 (0.00% of base)
    diff is a regression.


Top file regressions (bytes):
         564 : System.Linq.dasm (0.06% of base)
          78 : System.Linq.Expressions.dasm (0.01% of base)
          18 : System.Private.CoreLib.dasm (0.00% of base)

Top file improvements (bytes):
         -36 : System.Collections.dasm (-0.01% of base)
         -29 : System.Collections.Immutable.dasm (-0.00% of base)
         -26 : System.Text.Json.dasm (-0.00% of base)
         -20 : Microsoft.CodeAnalysis.dasm (-0.00% of base)

7 total files with Code Size differences (4 improved, 3 regressed), 264 unchanged.

Top method regressions (bytes):
         234 (45.97% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):Vector`1
         234 (45.97% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):Vector`1
          96 ( 4.37% of base) : System.Linq.Expressions.dasm - CollectionExtensions:ListHashCode(ReadOnlyCollection`1):int (7 methods)
          60 ( 1.00% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1,Func`2):long (14 methods)
          60 ( 1.00% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1,Func`2):long (14 methods)
          29 (19.21% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(double):bool:this
          26 (16.46% of base) : System.Collections.dasm - LinkedList`1:FindLast(double):LinkedListNode`1:this
          18 (23.08% of base) : System.Private.CoreLib.dasm - <>c:<CreateManifestString>b__18_1(KeyValuePair`2,KeyValuePair`2):int:this
          18 (13.43% of base) : System.Collections.dasm - LinkedList`1:Find(double):LinkedListNode`1:this
          13 ( 1.85% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):long (2 methods)
          13 ( 1.85% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):long (2 methods)
          12 ( 1.73% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):int (2 methods)
          12 ( 1.73% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):int (2 methods)
           5 ( 3.91% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(int):bool:this
           3 ( 2.31% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(long):bool:this
           2 ( 0.08% of base) : System.Text.Json.dasm - JsonPropertyInfo`1:GetMemberAndWriteJson(Object,byref,Utf8JsonWriter):bool:this (7 methods)
           1 ( 0.07% of base) : System.Collections.dasm - SortedDictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Remove(KeyValuePair`2):bool:this (7 methods)

Top method improvements (bytes):
         -43 (-2.13% of base) : System.Linq.Expressions.dasm - CollectionExtensions:ListEquals(ReadOnlyCollection`1,ReadOnlyCollection`1):bool (7 methods)
         -28 (-3.44% of base) : System.Text.Json.dasm - GenericMethodHolder`1:IsDefaultValue(Object):bool:this (7 methods)
         -20 (-12.35% of base) : Microsoft.CodeAnalysis.dasm - TextChange:Equals(TextChange):bool:this
         -19 (-13.97% of base) : System.Collections.dasm - LinkedList`1:FindLast(ubyte):LinkedListNode`1:this
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):ubyte
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):short
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):ubyte
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):short
         -14 (-8.28% of base) : System.Collections.dasm - LinkedList`1:FindLast(Vector`1):LinkedListNode`1:this
         -13 (-11.11% of base) : System.Collections.dasm - LinkedList`1:Find(ubyte):LinkedListNode`1:this
         -12 (-8.16% of base) : System.Collections.dasm - LinkedList`1:Find(Vector`1):LinkedListNode`1:this
         -10 (-7.52% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(ubyte):bool:this
          -8 (-5.84% of base) : System.Collections.dasm - LinkedList`1:FindLast(short):LinkedListNode`1:this
          -5 (-4.24% of base) : System.Collections.dasm - LinkedList`1:Find(short):LinkedListNode`1:this
          -5 (-1.44% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,double,long,long):bool
          -4 (-3.05% of base) : System.Collections.dasm - LinkedList`1:FindLast(int):LinkedListNode`1:this
          -4 (-3.01% of base) : System.Collections.dasm - LinkedList`1:FindLast(long):LinkedListNode`1:this
          -4 (-1.80% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,__Canon,long,long):bool
          -4 (-1.17% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,ubyte,long,long):bool
          -4 (-1.17% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,short,long,long):bool

Top method regressions (percentages):
         234 (45.97% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):Vector`1
         234 (45.97% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):Vector`1
          18 (23.08% of base) : System.Private.CoreLib.dasm - <>c:<CreateManifestString>b__18_1(KeyValuePair`2,KeyValuePair`2):int:this
          29 (19.21% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(double):bool:this
          26 (16.46% of base) : System.Collections.dasm - LinkedList`1:FindLast(double):LinkedListNode`1:this
          18 (13.43% of base) : System.Collections.dasm - LinkedList`1:Find(double):LinkedListNode`1:this
          96 ( 4.37% of base) : System.Linq.Expressions.dasm - CollectionExtensions:ListHashCode(ReadOnlyCollection`1):int (7 methods)
           5 ( 3.91% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(int):bool:this
           3 ( 2.31% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(long):bool:this
          13 ( 1.85% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):long (2 methods)
          13 ( 1.85% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):long (2 methods)
          12 ( 1.73% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):int (2 methods)
          12 ( 1.73% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):int (2 methods)
          60 ( 1.00% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1,Func`2):long (14 methods)
          60 ( 1.00% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1,Func`2):long (14 methods)
           2 ( 0.08% of base) : System.Text.Json.dasm - JsonPropertyInfo`1:GetMemberAndWriteJson(Object,byref,Utf8JsonWriter):bool:this (7 methods)
           1 ( 0.07% of base) : System.Collections.dasm - SortedDictionary`2:System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.Remove(KeyValuePair`2):bool:this (7 methods)

Top method improvements (percentages):
         -19 (-13.97% of base) : System.Collections.dasm - LinkedList`1:FindLast(ubyte):LinkedListNode`1:this
         -20 (-12.35% of base) : Microsoft.CodeAnalysis.dasm - TextChange:Equals(TextChange):bool:this
         -13 (-11.11% of base) : System.Collections.dasm - LinkedList`1:Find(ubyte):LinkedListNode`1:this
         -14 (-8.28% of base) : System.Collections.dasm - LinkedList`1:FindLast(Vector`1):LinkedListNode`1:this
         -12 (-8.16% of base) : System.Collections.dasm - LinkedList`1:Find(Vector`1):LinkedListNode`1:this
         -10 (-7.52% of base) : System.Linq.Expressions.dasm - ReadOnlyCollectionBuilder`1:Contains(ubyte):bool:this
          -8 (-5.84% of base) : System.Collections.dasm - LinkedList`1:FindLast(short):LinkedListNode`1:this
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):ubyte
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Max(IEnumerable`1):short
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):ubyte
         -18 (-4.72% of base) : System.Linq.dasm - Enumerable:Min(IEnumerable`1):short
          -5 (-4.24% of base) : System.Collections.dasm - LinkedList`1:Find(short):LinkedListNode`1:this
         -28 (-3.44% of base) : System.Text.Json.dasm - GenericMethodHolder`1:IsDefaultValue(Object):bool:this (7 methods)
          -4 (-3.05% of base) : System.Collections.dasm - LinkedList`1:FindLast(int):LinkedListNode`1:this
          -4 (-3.01% of base) : System.Collections.dasm - LinkedList`1:FindLast(long):LinkedListNode`1:this
         -43 (-2.13% of base) : System.Linq.Expressions.dasm - CollectionExtensions:ListEquals(ReadOnlyCollection`1,ReadOnlyCollection`1):bool (7 methods)
          -4 (-1.80% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,__Canon,long,long):bool
          -5 (-1.44% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,double,long,long):bool
          -4 (-1.23% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,int,long,long):bool
          -4 (-1.23% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:TryUpdate(byref,long,long,long):bool

45 total methods with Code Size differences (28 improved, 17 regressed), 261379 unchanged.

Size regressions/improvements because of inlining (some Equal/Compare implementations lead to size improvements after inlining, e.g. Byte.CompareTo)

EgorBo · 2021-02-16T18:30:59Z

Also add the test case from #10500 and look at diffs (thought I had added it, but looks like I didn't...)

@AndyAyersMS Are you sure that is a correct link?

AndyAyersMS · 2021-02-16T18:35:50Z

Try #10050.

AndyAyersMS

Changes look good.

Are you going to follow up by trying to enable the special DCE? If so, maybe open a new issue..?

EgorBo · 2021-02-16T19:46:13Z

Are you going to follow up by trying to enable the special DCE? If so, maybe open a new issue..?

Yes, will open a new issue for that in a moment.

AndyAyersMS · 2021-02-17T23:04:55Z

Did you get a chance to look at the impact of this change on the test you added (the one from #10050)?

How far are we from properly handling the cases there?

EgorBo · 2021-02-18T15:32:46Z

Did you get a chance to look at the impact of this change on the test you added (the one from #10050)?

How far are we from properly handling the cases there?

Yes, here the diff for all methods inside that test assembly: https://www.diffchecker.com/0QNbzcdW (left - master, right - this PR)

int Sink(int v0, int v1) and int GeneralSink<T>(T v0, T v1) (for T=int) are improved.

Move impGetSpecialIntrinsicExactReturnType to gtGetClassHandle

ef408e3

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 14, 2021

clean up

86dcbfd

Update gentree.cpp

526b46f

JulieLeeMSFT assigned EgorBo Feb 16, 2021

JulieLeeMSFT added this to the 6.0.0 milestone Feb 16, 2021

AndyAyersMS approved these changes Feb 16, 2021

View reviewed changes

EgorBo mentioned this pull request Feb 16, 2021

[RyuJIT] Drop redundant static initalizations #48358

Closed

Add a test

33c5e1a

EgorBo merged commit bc544bd into dotnet:master Feb 17, 2021

ghost locked as resolved and limited conversation to collaborators Mar 20, 2021

[RyuJIT] Track exact class for special intrinsics (EqulityComparer, Comparer) #48279

[RyuJIT] Track exact class for special intrinsics (EqulityComparer, Comparer) #48279

Conversation

EgorBo commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Feb 14, 2021

Uh oh!

benaadams commented Feb 14, 2021

Uh oh!

EgorBo commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Feb 15, 2021

Uh oh!

AndyAyersMS commented Feb 15, 2021

Uh oh!

AndyAyersMS commented Feb 15, 2021

Uh oh!

EgorBo commented Feb 16, 2021

Uh oh!

EgorBo commented Feb 16, 2021

Uh oh!

AndyAyersMS commented Feb 16, 2021

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Feb 16, 2021

Uh oh!

AndyAyersMS commented Feb 17, 2021

Uh oh!

EgorBo commented Feb 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EgorBo commented Feb 14, 2021 •

edited

Loading

EgorBo commented Feb 14, 2021 •

edited

Loading

EgorBo commented Feb 15, 2021 •

edited

Loading