Speed up fgLocalVarLiveness in minopts. #12665
Conversation
|
@dotnet-bot |
In particular, do not re-sort lclVars, run LVA, or run DSE.
|
dotnet-bot |
|
@BruceForstall @CarolEidt any thoughts? This is worth another couple percent on Bruce's dataset at the cost of never issuing early kills for lclVars. Note that this cost may actually be zero, as IIUC we do not issue early kills today due to Peter's |
It seems that this currently does only the first, unless I am missing something (since the code to disable liveness is under I think it looks good, and takes it a step further than my WIP/deferred work on simply disabling tracking of non-user lclVars. |
src/jit/liveness.cpp
Outdated
| @@ -1323,6 +1322,13 @@ class LiveVarAnalysis | |||
|
|
|||
| void Compiler::fgLiveVarAnalysis(bool updateInternalOnly) | |||
| { | |||
| #if CAN_DISABLE_DFA | |||
There was a problem hiding this comment.
Is there a case where we think CAN_DISABLE_DFA would be false?
Also, I think the condition below should be made consistent with what the register allocator is doing, i.e. it should be if(compiler->opts.compFlags & CLFLG_REGVAR) != 0) here, and presumably m_lsra->enregisterLocalVars in Lowering. Otherwise, we should simply set LinearScan::enregisterLocalVars to false always under minopts.
There was a problem hiding this comment.
Currently CAN_DISABLE_DFA is never false. We could probably just remove the symbol entirely--I only used it in because these changes depend on the minopts-only code in fgPerBlockLocalVarLiveness that is also conditional on this symbol.
I agree that we should keep this consistent with the allocator's decisions, though I think that minopts should remain a part of the condition as liveness is also used by DSE and the emitter. I think the appropriate condition under which to disable these operations is if (opts.MinOpts() && !enregisterLocalVars).
There was a problem hiding this comment.
Wow - I didn't realize there was a CAN_DISABLE_DFA symbol!
I agree that we should keep this consistent with the allocator's decisions, though I think that minopts should remain a part of the condition as liveness is also used by DSE and the emitter. I think the appropriate condition under which to disable these operations is if (opts.MinOpts() && !enregisterLocalVars).
I agree about including minopts. Note, though, that enregisterLocalVars isn't available in liveness.
There was a problem hiding this comment.
Yeah, I was considering adding a parameter to fgLocalVarLiveness to communicate that info.
And cut out a little bit more of liveness that does not need to run if we're not enregistering lclVars.
|
@dotnet-bot |
src/jit/lower.cpp
Outdated
| @@ -4588,7 +4588,7 @@ void Lowering::DoPhase() | |||
| // For now we'll take the throughput hit of recomputing local liveness but in the long term | |||
| // we're striving to use the unified liveness computation (fgLocalVarLiveness) and stop | |||
| // computing it separately in LSRA. | |||
| if (comp->lvaCount != 0) | |||
| if ((comp->lvaCount != 0) && !comp->backendRequiresLocalVarLifetimes()) | |||
There was a problem hiding this comment.
Shouldn't this be if ((comp->lvaCount != 0) && comp->backendRequiresLocalVarLifetimes())?
|
@dotnet-bot |
pgavlin
changed the title
In particular:
mustInit