JIT: Propagate flow into finally regions correctly in synthesis #114016
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
What do we do for the likelihoods coming out of a finally? Assume each is equally likely? It might be better to weight the likelihoods based on the weights of the associated callfinallies. Eg if there are two callfinallies, A with weight 9 and B with weight 1, the edge from the retfinally to the continuation of A should have 0.9 likelihood. |
That seems to be the case: runtime/src/coreclr/jit/importer.cpp Line 12644 in 65f0139 Let me try your suggestion... |
|
/azp run runtime-coreclr libraries-pgo |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Aside from timeouts, I'm not seeing any failures in @AndyAyersMS PTAL. Aside from |
src/coreclr/jit/fgbasic.cpp
Outdated
| // If the block has other successors, distribute the removed edge's likelihood among the remaining successor edges. | ||
| if (succCount > 1) | ||
| { | ||
| const weight_t likelihoodIncrease = succEdge->getLikelihood() / (succCount - 1); |
There was a problem hiding this comment.
Shouldn't these proportionally scale up?
Say there are 3 successors with likelihoods A 0.1, B 0.1, C 0.8. We remove A. Then we should have B = 0.1111..., C = 0.8888...
With these changes we'd get B = 0.15, C = 0.85, so the relative likelihood of B would increase.
Generally
There was a problem hiding this comment.
Good point; fixed
src/coreclr/jit/fgbasic.cpp
Outdated
| } | ||
|
|
||
| // If the block has other successors, distribute the removed edge's likelihood among the remaining successor edges. | ||
| if (succCount > 1) |
|
/azp run runtime-coreclr libraries-pgo |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@AndyAyersMS re: the profile consistency issue I mentioned yesterday, it does seem to be a floating-point precision issue when computing cyclic probabilities. Here's the smallest example I could find: At some point, we gained some extra weight in the loop for which All three loops have one test block each, and there isn't any EH flow to contend with, so I expect the exit block I tweaked the entry/exit residual check to detect failed convergence for synthesis runs after importation (enabling this check beforehand causes diffs, as we'll blend likelihoods and resynthesize more often). This feels like a quirk: Another option is to mimic the method entry/exit consistency check for each loop body: If the loop doesn't have EH flow, then we'd expect its entry flow to match its exit flow. There's nothing stopping this imprecision from cropping up for loop bodies with EH though, so neither option seems robust. |
|
Attach the full log (at least the synthesis related part) if you get a chance. I wonder if we have some block whose likelihoods doesn't sum to 1.0, and this leads to the problem (though perhaps not, you would think this would lead us to underestimate the flow out of a loop)? In |
|
Here's the JitDump:
Tightening this invariant didn't reveal anything. |
|
Right, we should see BB34 be exactly 1.0 here, not 1.000791, since there is a single loop exit. But the backedge drops below 1.0 so we don't notice. I can't tell where things go wrong from the dump, perhaps because of rounding issues in the displayed values. BB18 is only reached via a chain of jumps, and BB19 has many preds, so likely one or both of those weights are slightly too high. |
src/coreclr/jit/fgbasic.cpp
Outdated
|
|
||
| // Recompute the likelihoods of the block's other successor edges. | ||
| const weight_t removedLikelihood = succEdge->getLikelihood(); | ||
| for (unsigned i = 0; (removedLikelihood != 1.0) && (i < (succCount - 1)); i++) |
There was a problem hiding this comment.
We still need to handle the case where removed likelihood is 1.0, don't we (probably rare)?
In that case we should just spread the likelihood equally.
src/coreclr/jit/fgbasic.cpp
Outdated
|
|
||
| // Recompute the likelihoods of the block's other successor edges. | ||
| const weight_t removedLikelihood = succEdge->getLikelihood(); | ||
| for (unsigned i = 0; (removedLikelihood != 1.0) && (i < (succCount - 1)); i++) |
| // If we removed all of the flow out of 'block', distribute flow among the remaining edges evenly. | ||
| const weight_t currLikelihood = succTab[i]->getLikelihood(); | ||
| const weight_t newLikelihood = currLikelihood / (1.0 - removedLikelihood); | ||
| const weight_t newLikelihood = |
There was a problem hiding this comment.
If we 're removing all the likelihood then currLikelihood for each survivor will be 0.
This needs to be 1 / succCount if removedLikelihood == 1.0.
There was a problem hiding this comment.
Sorry I blanked on this -- fixed
|
/ba-g blocked by timeouts |
2 participants
Part of #107749. The first profile synthesis run happens before importation, so we don't model flow in and out of finally regions with flow edges yet. As a workaround, synthesis gives each finally region the same weight as its corresponding try region. When call-finally pairs are created, the tail inherits the weight of its head under the (faulty) assumption that all flow into a call-finally will return to the same pair. Once we have flow edges, we can compute the flow out of finally regions the same way as we compute flow elsewhere. It's important that synthesis models flow through finally regions via flow edges once we have them, or else flow through a loop that executes a finally might be lost, messing up the cyclic probability computation and flattening the loop's weight.
I noticed this issue after discovering that profile synthesis can disable profile consistency checking if it messed up the profile under the assumption that incorrect IL can have nonsensical flow. In such cases, synthesis will disable profile checks until the importer has run, after which the checks will be re-enabled. This quirk is specific to the pre-importation run of synthesis, so later runs can quietly disable consistency checks indefinitely, hiding bugs in synthesis (such as its inability to handle finally regions).
I want to disable this quirk for post-importation runs of synthesis, but there's one more issue with synthesis I have to resolve first: I'm seeing instances where synthesis computes cyclic probabilities close to the cap, but not quite exceeding it. Thus, synthesis doesn't flag the profile as approximate, but consistency checks find that the flow exiting a loop exceeds the flow entering it. I'm not sure if lowering the likelihood cap is a sustainable or desirable fix for this -- perhaps some more sophisticated detection of approximate consistency would be better.