Reduce cost of checking OptimizationEnabled #80948
Conversation
I noticed that the MSVC compiler wasn't inlining some checks to OptimizationEnabled()/Disabled(), which should be super simple, although it's a small computed value. To help, cache the computed value so no computation need be done when checking it.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsI noticed that the MSVC compiler wasn't inlining some checks to OptimizationEnabled()/Disabled(), which should be super simple, although it's a small computed value. To help, cache the computed value so no computation need be done when checking it.
|
| // 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()) |
There was a problem hiding this comment.
!compiler->opts.MinOpts() && !compiler->opts.DbgCode()
This is doing the same thing if (opts.OptimizationsEnabled()) does. Should it just be simplified to that?
| // 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()) |
There was a problem hiding this comment.
Same here. This is really just if ((call->gtNext == nullptr) && compiler->opts.OptimizationsEnabled())
| bool MinOpts() const | ||
| { | ||
| assert(compMinOptsIsSet); | ||
| compMinOptsIsUsed = true; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Agree. Shouldn't compMinOptsIsSet sufficient to make sure that we are not calling MinOpts() before it is set?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Couldn't we just have a check in SetMinOpts to assert we aren't past import or anything?
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.
There was a problem hiding this comment.
Perhaps, but the effect would not be as obvious or direct, and of course, compMinOptsIsUsed is DEBUG-only code.
|
@BruceForstall I actually did this in #77465 that I planned to land this (well, next) week - OptimizationEnabled is a single field load in my PR. (well, except hiding compDbgCode under a method) |
|
TP diffs up to -0.10%. Odd small regression on x64 MinOpts collections.
Ok, I'll close this. I was curious what the TP change would be, and it's surprisingly significant (given our SPMI measurement). I presume you maintain that benefit. |
|
Yes it was around that. I promise I'll land it coming few days 🙂 |
ghost
locked as resolved and limited conversation to collaborators
4 participants
I noticed that the MSVC compiler wasn't inlining some checks to OptimizationEnabled()/Disabled(), which should be super simple, although it's a small computed value. To help, cache the computed value so no computation need be done when checking it.