-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Reduce cost of checking OptimizationEnabled #80948
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
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 |
|---|---|---|
|
|
@@ -7084,7 +7084,7 @@ void CodeGen::genCall(GenTreeCall* call) | |
|
|
||
| // If there is nothing next, that means the result is thrown away, so this value is not live. | ||
| // However, for minopts or debuggable code, we keep it live to support managed return value debugging. | ||
| if ((call->gtNext == nullptr) && !compiler->opts.MinOpts() && !compiler->opts.compDbgCode) | ||
| if ((call->gtNext == nullptr) && !compiler->opts.MinOpts() && !compiler->opts.DbgCode()) | ||
|
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. Same here. This is really just |
||
| { | ||
| gcInfo.gcMarkRegSetNpt(RBM_INTRET); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9191,39 +9191,61 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |
| // Maximum number of locals before turning off the inlining | ||
| #define MAX_LV_NUM_COUNT_FOR_INLINING 512 | ||
|
|
||
| private: | ||
| bool compMinOpts; | ||
| bool compMinOptsIsSet; | ||
| bool compDbgCode; // Generate debugger-friendly code? | ||
| bool compOptimizationEnabled; // A cached, composite value | ||
|
|
||
| #ifdef DEBUG | ||
| mutable bool compMinOptsIsUsed; | ||
| #endif // !DEBUG | ||
|
|
||
| void SetOptimizationEnabled() | ||
| { | ||
| // We want to be careful with the `compMinOptsIsSet` and `compMinOptsIsUsed` values. | ||
| // Caching the value shouldn't touch these. However, using this value should have the | ||
| // same asserts as using `MinOpts()`. | ||
| compOptimizationEnabled = !compMinOpts && !compDbgCode; | ||
| } | ||
|
|
||
| public: | ||
| void InitializeMinOpts() | ||
| { | ||
| INDEBUG(compMinOptsIsUsed = false); | ||
| compMinOptsIsSet = false; | ||
| } | ||
|
|
||
| #ifdef DEBUG | ||
| bool MinOpts() const | ||
| { | ||
| assert(compMinOptsIsSet); | ||
| compMinOptsIsUsed = true; | ||
|
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. Just wondering, what's the general point of this and why do we need to track it as part of the check whether min opts is enabled
Contributor
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. Agree. Shouldn't
Contributor
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. Once we read the value, we should never try to set it afterwards -- someone has taken a dependency on the value. Like the PhasedVar stuff.
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. Couldn't we just have a check in We should really only be setting it when the compiler is created or in the case we fault and fallback to minopts, right? Once we've hit import, we're basically guaranteed to have some path that checks it.
Contributor
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. Perhaps, but the effect would not be as obvious or direct, and of course, |
||
| return compMinOpts; | ||
| } | ||
| bool IsMinOptsSet() const | ||
| { | ||
| return compMinOptsIsSet; | ||
| } | ||
| #else // !DEBUG | ||
| bool MinOpts() const | ||
| { | ||
| return compMinOpts; | ||
| } | ||
| #endif // !DEBUG | ||
|
|
||
| bool IsMinOptsSet() const | ||
| { | ||
| return compMinOptsIsSet; | ||
| } | ||
| #endif // !DEBUG | ||
|
|
||
| bool OptimizationDisabled() const | ||
| { | ||
| return MinOpts() || compDbgCode; | ||
| assert(compMinOptsIsSet); | ||
| INDEBUG(compMinOptsIsUsed = true); | ||
| return !compOptimizationEnabled; | ||
| } | ||
| bool OptimizationEnabled() const | ||
| { | ||
| return !OptimizationDisabled(); | ||
| assert(compMinOptsIsSet); | ||
| INDEBUG(compMinOptsIsUsed = true); | ||
| return compOptimizationEnabled; | ||
| } | ||
|
|
||
| void SetMinOpts(bool val) | ||
|
|
@@ -9232,6 +9254,20 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |
| assert(!compMinOptsIsSet || (compMinOpts == val)); | ||
| compMinOpts = val; | ||
| compMinOptsIsSet = true; | ||
|
|
||
| SetOptimizationEnabled(); // Update compOptimizationEnabled | ||
| } | ||
|
|
||
| bool DbgCode() const | ||
| { | ||
| return compDbgCode; | ||
| } | ||
|
|
||
| void SetDbgCode(bool val) | ||
| { | ||
| compDbgCode = val; | ||
|
|
||
| SetOptimizationEnabled(); // Update compOptimizationEnabled | ||
| } | ||
|
|
||
| // true if the CLFLG_* for an optimization is set. | ||
|
|
@@ -9325,7 +9361,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |
| } | ||
|
|
||
| bool compScopeInfo; // Generate the LocalVar info ? | ||
| bool compDbgCode; // Generate debugger-friendly code? | ||
| bool compDbgInfo; // Gather debugging info? | ||
| bool compDbgEnC; | ||
|
|
||
|
|
@@ -9932,7 +9967,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |
| if (opts.IsOSR()) | ||
| return false; | ||
| #endif | ||
| return !info.compInitMem && opts.compDbgCode; | ||
| return !info.compInitMem && opts.DbgCode(); | ||
| } | ||
|
|
||
| // Returns true if the jit supports having patchpoints in this method. | ||
|
|
||
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.
!compiler->opts.MinOpts() && !compiler->opts.DbgCode()This is doing the same thing
if (opts.OptimizationsEnabled())does. Should it just be simplified to that?