-
Notifications
You must be signed in to change notification settings - Fork 5.4k
JIT: revise how the jit tracks use of generics context #34827
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
Changes from all commits
63e4e2f
5ca4a17
c709e93
655f972
8ee5fc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1938,9 +1938,9 @@ inline bool Compiler::lvaKeepAliveAndReportThis() | |
| if (opts.compDbgCode) | ||
| return true; | ||
|
|
||
| if (lvaGenericsContextUseCount > 0) | ||
| if (lvaGenericsContextInUse) | ||
| { | ||
| JITDUMP("Reporting this as generic context: %u refs\n", lvaGenericsContextUseCount); | ||
| JITDUMP("Reporting this as generic context\n"); | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -1951,14 +1951,11 @@ inline bool Compiler::lvaKeepAliveAndReportThis() | |
| // because collectible types need the generics context when gc-ing. | ||
| if (genericsContextIsThis) | ||
| { | ||
| const bool isUsed = lvaGenericsContextUseCount > 0; | ||
| const bool mustKeep = (info.compMethodInfo->options & CORINFO_GENERICS_CTXT_KEEP_ALIVE) != 0; | ||
|
|
||
| if (isUsed || mustKeep) | ||
| if (lvaGenericsContextInUse || mustKeep) | ||
| { | ||
| JITDUMP("Reporting this as generic context: %u refs%s\n", lvaGenericsContextUseCount, | ||
| mustKeep ? ", must keep" : ""); | ||
|
|
||
| JITDUMP("Reporting this as generic context: %s\n", mustKeep ? "must keep" : "referenced"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You kept the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. |
||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -1986,7 +1983,7 @@ inline bool Compiler::lvaReportParamTypeArg() | |
|
|
||
| // Otherwise, if an exact type parameter is needed in the body, report the generics context. | ||
| // We do this because collectible types needs the generics context when gc-ing. | ||
| if (lvaGenericsContextUseCount > 0) | ||
| if (lvaGenericsContextInUse) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,7 +39,7 @@ void Compiler::lvaInit() | |||||||||||||||||||||||||
| /* We haven't allocated stack variables yet */ | ||||||||||||||||||||||||||
| lvaRefCountState = RCS_INVALID; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| lvaGenericsContextUseCount = 0; | ||||||||||||||||||||||||||
| lvaGenericsContextInUse = false; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| lvaTrackedToVarNumSize = 0; | ||||||||||||||||||||||||||
| lvaTrackedToVarNum = nullptr; | ||||||||||||||||||||||||||
|
|
@@ -3606,6 +3606,8 @@ var_types LclVarDsc::lvaArgType() | |||||||||||||||||||||||||
| // eligible for assertion prop, single defs, and tracking which blocks | ||||||||||||||||||||||||||
| // hold uses. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // Looks for uses of generic context and sets lvaGenericsContextInUse. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // In checked builds: | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // Verifies that local accesses are consistenly typed. | ||||||||||||||||||||||||||
|
|
@@ -3711,6 +3713,17 @@ void Compiler::lvaMarkLclRefs(GenTree* tree, BasicBlock* block, Statement* stmt, | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /* This must be a local variable reference */ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // See if this is a generics context use. | ||||||||||||||||||||||||||
| if ((tree->gtFlags & GTF_VAR_CONTEXT) != 0) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| assert(tree->OperIs(GT_LCL_VAR)); | ||||||||||||||||||||||||||
| if (!lvaGenericsContextInUse) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| JITDUMP("-- generic context in use at [%06u]\n", dspTreeID(tree)); | ||||||||||||||||||||||||||
| lvaGenericsContextInUse = true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| assert((tree->gtOper == GT_LCL_VAR) || (tree->gtOper == GT_LCL_FLD)); | ||||||||||||||||||||||||||
| unsigned lclNum = tree->AsLclVarCommon()->GetLclNum(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -4009,24 +4022,26 @@ void Compiler::lvaMarkLocalVars() | |||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #if ASSERTION_PROP | ||||||||||||||||||||||||||
| assert(opts.OptimizationEnabled()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Note: optAddCopies() depends on lvaRefBlks, which is set in lvaMarkLocalVars(BasicBlock*), called above. | ||||||||||||||||||||||||||
| optAddCopies(); | ||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||
| const bool reportParamTypeArg = lvaReportParamTypeArg(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Update bookkeeping on the generic context. | ||||||||||||||||||||||||||
| if (lvaKeepAliveAndReportThis()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| lvaTable[0].lvImplicitlyReferenced = 1; | ||||||||||||||||||||||||||
| // This isn't strictly needed as we will make a copy of the param-type-arg | ||||||||||||||||||||||||||
| // in the prolog. However, this ensures that the LclVarDsc corresponding to | ||||||||||||||||||||||||||
| // info.compTypeCtxtArg is valid. | ||||||||||||||||||||||||||
| lvaGetDesc(0u)->lvImplicitlyReferenced = reportParamTypeArg; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| else if (lvaReportParamTypeArg()) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| lvaTable[info.compTypeCtxtArg].lvImplicitlyReferenced = 1; | ||||||||||||||||||||||||||
| // We should have a context arg. | ||||||||||||||||||||||||||
| assert(info.compTypeCtxtArg != BAD_VAR_NUM); | ||||||||||||||||||||||||||
| lvaGetDesc(info.compTypeCtxtArg)->lvImplicitlyReferenced = reportParamTypeArg; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update. |
||||||||||||||||||||||||||
| #if ASSERTION_PROP | ||||||||||||||||||||||||||
| assert(opts.OptimizationEnabled()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Note: optAddCopies() depends on lvaRefBlks, which is set in lvaMarkLocalVars(BasicBlock*), called above. | ||||||||||||||||||||||||||
| optAddCopies(); | ||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| //------------------------------------------------------------------------ | ||||||||||||||||||||||||||
|
|
@@ -4046,7 +4061,10 @@ void Compiler::lvaMarkLocalVars() | |||||||||||||||||||||||||
| // In fast-jitting modes where we don't ref count locals, this bypasses | ||||||||||||||||||||||||||
| // actual counting, and makes all locals implicitly referenced on first | ||||||||||||||||||||||||||
| // compute. It asserts all locals are implicitly referenced on recompute. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // When optimizing we also recompute lvaGenericsContextInUse based | ||||||||||||||||||||||||||
| // on specially flagged LCL_VAR appearances. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| JITDUMP("\n*** lvaComputeRefCounts ***\n"); | ||||||||||||||||||||||||||
|
|
@@ -4141,6 +4159,11 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) | |||||||||||||||||||||||||
| varDsc->lvSingleDef = varDsc->lvIsParam; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Remember current state of generic context use, and prepare | ||||||||||||||||||||||||||
| // to compute new state. | ||||||||||||||||||||||||||
| const bool oldLvaGenericsContextInUse = lvaGenericsContextInUse; | ||||||||||||||||||||||||||
| lvaGenericsContextInUse = false; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| JITDUMP("\n*** lvaComputeRefCounts -- explicit counts ***\n"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Second, account for all explicit local variable references | ||||||||||||||||||||||||||
|
|
@@ -4176,6 +4199,12 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) | |||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| varDsc->incRefCnts(weight, this); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if ((node->gtFlags & GTF_VAR_CONTEXT) != 0) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| assert(node->OperIs(GT_LCL_VAR)); | ||||||||||||||||||||||||||
| lvaGenericsContextInUse = true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -4190,6 +4219,21 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers) | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (oldLvaGenericsContextInUse && !lvaGenericsContextInUse) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| // Context was in use but no longer is. This can happen | ||||||||||||||||||||||||||
| // if we're able to optimize, so just leave a note. | ||||||||||||||||||||||||||
| JITDUMP("\n** Generics context no longer in use\n"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| else if (lvaGenericsContextInUse && !oldLvaGenericsContextInUse) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| // Context was not in use but now is. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // Changing from unused->used should never happen; creation of any new IR | ||||||||||||||||||||||||||
| // for context use should also be settting lvaGenericsContextInUse. | ||||||||||||||||||||||||||
| assert(!"unexpected new use of generics context"); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| JITDUMP("\n*** lvaComputeRefCounts -- implicit counts ***\n"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Third, bump ref counts for some implicit prolog references | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: You are using "generics context" and "generic context" inconsistently in var names and comments in this change. I think it should be "generic context".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many many uses of "generics context" so it seems simplest to standardize on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm many votes in both camps. There are 137 case-insensitive uses of
and 77 uses of
The "S" variant appears in enums in corinfo.h so can't easily be updated. But we can fix other appearances if you think it's worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not worth cleaning this up in this PR.