Convert more jit phases to opt into common post phase checks #74041
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThere are 20-odd phases that haven't yet been converted to running the common post phase checks. This converts the first half of them. Note this is a bit of a lie. We actually don't run most checks until morph, but a future change will work on enabling checks throughout the phase list. I also did a bit of cleanup here and there; most notably we no longer needed Contributes to #2109.
|
|
@dotnet/jit-contrib PTAL |
|
|
||
| if (returnSpillVarDsc->lvSingleDef) | ||
| { | ||
| lvaUpdateClass(lvaInlineeReturnSpillTemp, retExprClassHnd, impInlineInfo->retExprClassHndIsExact); |
There was a problem hiding this comment.
It doesn't look like this change factors into the decision about returning MODIFIED_NOTHING below.
Also, presumably doing this before this phase is equivalent to doing it after, where it was being done before?
There was a problem hiding this comment.
I haven't been trying to track modifications that we don't check and/or dump anywhere.
| { | ||
| #ifdef DEBUG | ||
| if (verbose) | ||
| if (verbose && !isPhase) |
There was a problem hiding this comment.
(not related to this line)
Interesting that we don't consider removing BBF_REMOVED blocks as modified but I guess that makes sense.
src/coreclr/jit/fgprofile.cpp
Outdated
| } | ||
|
|
||
| //------------------------------------------------------------- | ||
| // fgComputeMissingBlockWeights: determine weights for blocks | ||
| // that were not profiled and do not yet have weights. | ||
| // | ||
| // Arguments | ||
| // return weight [out] - sum of weights for all return and throw blocks |
There was a problem hiding this comment.
nit
| // return weight [out] - sum of weights for all return and throw blocks | |
| // returnWeight [out] - sum of weights for all return and throw blocks |
src/coreclr/jit/flowgraph.cpp
Outdated
| fgVerifyHandlerTab(); | ||
| fgDebugCheckBBlist(); | ||
| #endif // DEBUG | ||
| return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; |
There was a problem hiding this comment.
Instead of tracking madeChanges you could just use:
| return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; | |
| return (compHndBBtabCount > 0) ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; |
| @@ -221,7 +223,7 @@ void Phase::PostPhase(PhaseStatus status) | |||
| // cse | |||
| // assertion prop | |||
| // range check | |||
| // update flow | |||
| // PHASE_OPT_UPDATE_FLOW_GRAPH, | |||
There was a problem hiding this comment.
Do you want to un-comment this?
There was a problem hiding this comment.
I had it that way at one point, but it caused loop table validity asserts.
Will revisit as part of round 2 when I try converting the rest of the phases.
|
SPMI failures are likely from #64497, will merge up next time I push. |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
All the jit stress failures are arm/arm64 infra issues. SPMI are timeouts / and or rerun failures because of pre-existing logs. Going to roll the dice here and say there's no reason to try and get all those green. |
ghost
locked as resolved and limited conversation to collaborators
There are 20-odd phases that haven't yet been converted to running the common post phase checks. This converts the first half of them.
Note this is a bit of a lie. We actually don't run most checks until morph, but a future change will work on enabling checks throughout the phase list.
I also did a bit of cleanup here and there; most notably we no longer needed
fgResetImplicitByRefRefCountas it was just setting the counts the values they already had.Contributes to #2109.