JIT: initial workup for suppressing dumps and checks post-phase#33305
JIT: initial workup for suppressing dumps and checks post-phase#33305AndyAyersMS merged 6 commits intodotnet:masterfrom
Conversation
|
@dotnet/jit-contrib curious what you think of this approach. If it looks reasonable I will propagate to more phases. Might be overkill to return fine-grained |
|
@dotnet/jit-contrib any thoughts on this proposed change to phases? |
sandreenko
left a comment
There was a problem hiding this comment.
I like the idea in general, I am not sure how much will it affect log sizes and checked TP.
Probably nobody is scrolling through the whole JitDump, but if you are looking for a specific tree and you will get twice less occurrences it will speed up the process significantly.
src/coreclr/src/jit/compiler.h
Outdated
There was a problem hiding this comment.
Nit: enum class should not need a class prefix, because they can't be used without className.
There was a problem hiding this comment.
Good point, it is annoying to repeat this.
CarolEidt
left a comment
There was a problem hiding this comment.
Looks like a good approach to me.
src/coreclr/src/jit/rationalize.cpp
Outdated
There was a problem hiding this comment.
Any idea what checking such an "other" category might want to trigger?
There was a problem hiding this comment.
Not sure? Seems like a good part of the compiler object state probably can't be checked effectively. But maybe there are some things we can come up with.
BruceForstall
left a comment
There was a problem hiding this comment.
LGTM.
Seems semi-dangerous to trust phases to report what changed and then not check. Seems ok to not dump in that case.
Allow phases to declare what parts of jit state they might have modified, and suppress dumps and checks based on these declarations. Existing phases that don't know about this are handled by defaulting to reporting that everything might have beem nodified. Changed over all explicit phases to return status, as well as a handful of phases that are methods on the Compiler object. Also converted a few lambda phases into Compiler phase methods.
f90713e to
277f289
Compare
|
Need to merge in latest... |
|
Moving out of draft status. @dotnet/jit-contrib any other thoughts? |
| @@ -24151,16 +24180,18 @@ void Compiler::fgRemoveEmptyFinally() | |||
| printf("\n"); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
GitHub bug: All the real changes are not shown
| const bool madeChanges = (status != PhaseStatus::MODIFIED_NOTHING); | ||
| const char* const statusMessage = madeChanges ? "" : " [no changes]"; | ||
| bool doPostPhase = false; | ||
|
|
There was a problem hiding this comment.
As a additional check if a phase reports MODIFIED_NOTHING, we could assert that no new Nodes were created.
assert(madeChanges || (compGenTreeID == s_previousValue));
There was a problem hiding this comment.
Good idea. I'll implement this, and also check that some other easy to verify things are unchanged.
|
Am going to ignore the OSX libraries test failure, suspect this is when we briefly lost connectivity to the test machines. |
|
Part of #2109. |
Allow phases to declare what parts of jit state they might have modified,
and suppress dumps and checks based on these declarations.
Existing phases that don't know about this are handled by defaulting to
reporting that everything might have beem nodified.