-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Convert more jit phases to opt into common post phase checks #74041
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
Conversation
|
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 |
src/coreclr/jit/fgopt.cpp
Outdated
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been trying to track modifications that we don't check and/or dump anywhere.
src/coreclr/jit/fgopt.cpp
Outdated
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.
(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
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.
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
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.
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; |
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.
Sure.
src/coreclr/jit/phase.cpp
Outdated
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.
Do you want to un-comment this?
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.
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. |
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.