-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add edge likelihood dumping; fix one edge likelihood update case #99740
Conversation
1. Add dumping of edge likelihood numbers to the block table and other block dumping. Examples: `BB17(0.143)`, `BB18(1)`. Edge likelihood dumping is parameterized in the code, but is currently always enabled. 2. Update `setLikelihood` and `updateLikelihood` to print the previous value of the likelihood that is being updated (if there is a previous value). 3. Fix a case of likelihood updating in `fgReplaceJumpTarget()`: in a switch block, when the new target block is already a target of the switch, then there already exists an edge from the switch to the new target. In that case, we were updating the edge dup count, but `fgAddRefPred` wasn't updating the edge likelihood to add the removed edge likelihood to the existing edge likelihood. This was found as part of work to fix JitOptRepeat: dotnet#94250. 4. Add natvis debugger support for FlowEdge. 5. In 'fgDebugCheckOutgoingProfileData()`, if outgoing likelihoods are invalid, print out all the likelihoods of the outgoing edges.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@amanasifkhalid @AndyAyersMS PTAL |
Here's a bigger example:
in the full IR dump, you'll see things like:
|
This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.
Thanks for fixing this—it sounds exactly like the bug I hit in libraries jitstress in #99628. |
JITDUMP("Created new exit " FMT_BB " to replace " FMT_BB " exit for " FMT_LP "\n", newExit->bbNum, exit->bbNum, | ||
loop->GetIndex()); |
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.
optSetWeightForPreheaderOrExit
references the block created, so moving its dumping here makes the jitdump look a bit strange I think.
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 added the "Canonicalize exit" line above so it's obvious what process is starting when new blocks start getting created -- before I was confused at why new blocks were getting created. So moving this line to the end "brackets" the process by telling you what happened when it's done. E.g.,
Canonicalize exit BB15 for L00 to have only loop predecessors
New Basic Block BB18 [0116] created.
Setting edge weights for BB18 -> BB15 to [0 .. 3.402823e+38]
setting likelihood of BB18 -> BB15 to 1
Estimated likelihood BB04 -> BB18 to be 0.6585233 (contribution: 35.62623)
Setting edge weights for BB04 -> BB18 to [35.62623 .. 35.62623]
Estimated likelihood BB05 -> BB18 to be 0.3993941 (contribution: 10.80365)
Setting edge weights for BB05 -> BB18 to [10.80365 .. 10.80365]
Setting edge weights for BB18 -> BB15 to [46.42988 .. 46.42988]
Created new exit BB18 to replace BB15 exit for L00
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.
Ok, seems fine -- I think you should change it in optCreatePreheader
as well then.
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.
LGTM, thanks!
assert(newEdge->getSourceBlock() == block); | ||
assert(newEdge->getDestinationBlock() == newTarget); | ||
|
||
if (newEdge->hasLikelihood() && oldEdge->hasLikelihood()) |
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 wonder if we can start skipping these checks and asserting that the edges have likelihoods already. Once Andy's last PR for propagating likelihoods is merged in, I'll take a look at removing these.
Failures are known or infra. |
BB17(0.143)
,BB18(1)
.Edge likelihood dumping is parameterized in the code, but is currently always enabled.
Update
setLikelihood
andupdateLikelihood
to print the previous value of the likelihood that is being updated (if there is a previous value).Fix a case of likelihood updating in
fgReplaceJumpTarget()
: in a switch block, when the new target block is already a target of the switch, then there already exists an edge from the switch to the new target. In that case, we were updating the edge dup count, butfgAddRefPred
wasn't updating the edge likelihood to add the removed edge likelihood to the existing edge likelihood.This was found as part of work to fix JitOptRepeat: #94250.
Add natvis debugger support for FlowEdge.
In 'fgDebugCheckOutgoingProfileData()`, if outgoing likelihoods are invalid, print out all the likelihoods of the outgoing edges.