Implement the JIT part of the dynamic dictionary expansion feature. by sandreenko · Pull Request #31957 · dotnet/runtime (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
Looks generally good to me, but I don't know much about this codebase to give any meaningful feedback. Thanks @sandreenko for implementing this!
| DEBUG_DESTROY_NODE(colonNullCheck); |
|---|
| // ((sizeCheck fails | |
| // Add checks and the handle as call arguments, indirect call transformer will handle this. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -116,6 +116,13 @@ class IndirectCallTransformer |
|---|
| transformer.Run(); |
| count++; |
| } |
| if (ContainsExpRuntimeLookup(stmt)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I can do that with my portion of the changes.
PR was updated, please take another look.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; left one small suggestion.
| { |
|---|
| GuardedDevirtualizationTransformer transformer(compiler, block, stmt); |
| transformer.Run(); |
| count++; |
| } |
| if (ContainsExpRuntimeLookup(stmt)) |
| else if (ContainsExpRuntimeLookup(stmt)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you'd do something like this:
if (doesMethodHaveExpRuntimeLookup() && ContainsExpRuntimeLookup(stmt))
(and similarly for the other clauses)
| DEBUG_DESTROY_NODE(colonNullCheck); |
|---|
| // ((sizeCheck fails | |
| // Add checks and the handle as call arguments, indirect call transformer will handle this. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This was referenced
Feb 12, 2020
Labels
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI