Implement the JIT part of the dynamic dictionary expansion feature. #31957

sandreenko · 2020-02-08T01:15:50Z

We would like to merge JIT changes first to allow @fadimounir pass tests when he adds VM changes in his PR.

The feature is described here.

The feature uses the pre-existing mechanism of introducing CFG in IndirectCallTransformer phase after importation.

The jit changes were tested with Fadi's VM changes and tests.

PTAL @dotnet/jit-contrib, @fadimounir

src/coreclr/src/jit/indirectcalltransformer.cpp

fadimounir · 2020-02-08T01:25:31Z

Looks generally good to me, but I don't know much about this codebase to give any meaningful feedback. Thanks @sandreenko for implementing this!

src/coreclr/src/jit/compiler.cpp

src/coreclr/src/jit/gentree.h

src/coreclr/src/jit/indirectcalltransformer.cpp

src/coreclr/src/jit/importer.cpp

AndyAyersMS · 2020-02-08T02:32:47Z

src/coreclr/src/jit/importer.cpp

+        DEBUG_DESTROY_NODE(colonNullCheck);
+
+        // ((sizeCheck fails || nullCheck fails))) ? (helperCall : handle).
+        // Add checks and the handle as call arguments, indirect call transformer will handle this.


Seems a little odd to me to model this as a call and not some new IR node type/oper that has the call and other nodes as children.

A new node/oper would be more obvious to check for later if it didn't get transformed for some reason; the call you produce here still looks like legal IR.

In the first prototype, I used double QMARK to get a valid IR from importer, but it required more changes in indirect call transformer (for example rename it to smth not call dependent).
And also, I had to destroy these QMARK in that phase, which was not free.

I am not a fan of adding new GenTree opcodes with such a short lifetime, but we can discuss it.

I think we're going to have more and more of these early-jit-phase only opcodes going forward, and may well want to defer their expansion.

It is often easier to reason about a runtime construct abstractly than reason about what its expansion does (say we want to CSE these lookups -- it will be tricky to do once they've introduced control flow).

I don't want to hold up this PR over this point, we can always come back later and change this aspect, when we've got more motivation to do so.

AndyAyersMS · 2020-02-08T02:35:21Z

src/coreclr/src/jit/indirectcalltransformer.cpp

@@ -8,8 +8,8 @@
 #endif

 // The IndirectCallTransformer transforms indirect calls that involve fat function
-// pointers or guarded devirtualization candidates. These transformations introduce
-// control flow and so can't easily be done in the importer.
+// pointers, guarded devirtualization candidates, or runtime lookup with dynamic dictionary expansion feature.


We should rename this base class and file, but maybe not just yet.

I have some work in progress that extends this too. Plenty of time to bikeshed on a new name.

AndyAyersMS · 2020-02-08T02:43:33Z

src/coreclr/src/jit/indirectcalltransformer.cpp

@@ -116,6 +116,13 @@ class IndirectCallTransformer
                transformer.Run();
                count++;
            }
+
+            if (ContainsExpRuntimeLookup(stmt))


Presumably only one of these Contains... can be true since we require the victim to be at or near the root of the tree. Would it make sense to early out when one fires?

Do we know for sure statement iteration is not messed up by these transformations?

We can exclude drilling into cases here by first looking at the various method flags again, right? Might save a bit of time. That is, if OMF_HAS_FATPOINTER is not set, no need to call ContainsFatCalli, and so on.

Would it make sense to early out when one fires?

It won't hurt, added.

I think if we are planning to add more and want to optimize than we probably should keep lists of statements that need transformations in Compiler class.

MichalStrehovsky · 2020-02-10T09:05:00Z

src/coreclr/src/jit/importer.cpp

@@ -2084,6 +2085,15 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken

        if (pRuntimeLookup->offsets[i] != 0)
        {
+// The last indirection could be subject to a size check (dynamic dictionary expansion feature)
+#if 0  // Uncomment that block when you add sizeOffset field to pRuntimeLookup.
+            if (i == pRuntimeLookup->indirections - 1 && pRuntimeLookup->sizeOffset != 0xFFFF)


Should the 0xFFFF be a #define in CorInfo.h? (We don't need to add that now, but it might be good to put something like CORINFO_LOOKUP_DIRECT_OFFSET here instead of the 0xFFFF so that it forces the addition when sizeOffset is added and this block is uncommented.

Yep. I can do that with my portion of the changes.

sandreenko · 2020-02-11T22:33:41Z

PR was updated, please take another look.

AndyAyersMS

Looks good; left one small suggestion.

AndyAyersMS · 2020-02-11T23:48:45Z

src/coreclr/src/jit/indirectcalltransformer.cpp

            {
                GuardedDevirtualizationTransformer transformer(compiler, block, stmt);
                transformer.Run();
                count++;
            }
-
-            if (ContainsExpRuntimeLookup(stmt))
+            else if (ContainsExpRuntimeLookup(stmt))


I was thinking you'd do something like this:

if (doesMethodHaveExpRuntimeLookup() && ContainsExpRuntimeLookup(stmt))

(and similarly for the other clauses)

AndyAyersMS · 2020-02-12T00:00:10Z

src/coreclr/src/jit/importer.cpp

+        DEBUG_DESTROY_NODE(colonNullCheck);
+
+        // ((sizeCheck fails || nullCheck fails))) ? (helperCall : handle).
+        // Add checks and the handle as call arguments, indirect call transformer will handle this.


I think we're going to have more and more of these early-jit-phase only opcodes going forward, and may well want to defer their expansion.

It is often easier to reason about a runtime construct abstractly than reason about what its expansion does (say we want to CSE these lookups -- it will be tricky to do once they've introduced control flow).

I don't want to hold up this PR over this point, we can always come back later and change this aspect, when we've got more motivation to do so.

Implement the JIT part of the dynamic dictionary expansion feature.

8efad58

sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 8, 2020

sandreenko requested a review from AndyAyersMS February 8, 2020 01:15

fadimounir reviewed Feb 8, 2020

View reviewed changes

src/coreclr/src/jit/indirectcalltransformer.cpp Outdated Show resolved Hide resolved

jkotas reviewed Feb 8, 2020

View reviewed changes

src/coreclr/src/jit/compiler.cpp Outdated Show resolved Hide resolved

jkotas reviewed Feb 8, 2020

View reviewed changes

src/coreclr/src/jit/gentree.h Outdated Show resolved Hide resolved

jkotas reviewed Feb 8, 2020

View reviewed changes

src/coreclr/src/jit/indirectcalltransformer.cpp Outdated Show resolved Hide resolved

jkotas reviewed Feb 8, 2020

View reviewed changes

src/coreclr/src/jit/indirectcalltransformer.cpp Outdated Show resolved Hide resolved

AndyAyersMS reviewed Feb 8, 2020

View reviewed changes

review response.

5944bcf

MichalStrehovsky reviewed Feb 10, 2020

View reviewed changes

AndyAyersMS approved these changes Feb 12, 2020

View reviewed changes

response review

d8f3895

This was referenced Feb 12, 2020

System.Net.NetworkInformation.Functional.Tests failing CI and PR #32179

Closed

Test failure: System.Net.NetworkInformation.Tests.NetworkInterfaceBasicTest/BasicTest_AccessInstanceProperties_NoExceptions_Linux #18090

Closed

sandreenko merged commit a06b447 into dotnet:master Feb 13, 2020

sandreenko deleted the MakeDectionaryLayoutDynamicJitPartOnly branch February 13, 2020 00:24

fadimounir mentioned this pull request Feb 14, 2020

Dynamic generic dictionary expansion feature #32270

Merged

1 task

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the JIT part of the dynamic dictionary expansion feature. #31957

Implement the JIT part of the dynamic dictionary expansion feature. #31957

sandreenko commented Feb 8, 2020

fadimounir commented Feb 8, 2020

AndyAyersMS Feb 8, 2020

sandreenko Feb 10, 2020

AndyAyersMS Feb 12, 2020

AndyAyersMS Feb 8, 2020

AndyAyersMS Feb 8, 2020

sandreenko Feb 10, 2020

MichalStrehovsky Feb 10, 2020

fadimounir Feb 13, 2020

sandreenko commented Feb 11, 2020

AndyAyersMS left a comment

AndyAyersMS Feb 11, 2020

AndyAyersMS Feb 12, 2020

Implement the JIT part of the dynamic dictionary expansion feature. #31957

Implement the JIT part of the dynamic dictionary expansion feature. #31957

Conversation

sandreenko commented Feb 8, 2020

fadimounir commented Feb 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Feb 11, 2020

AndyAyersMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment