JIT: one phi arg per pred #85546
JIT: one phi arg per pred #85546
Conversation
Revise SSA to add one phi arg per pred instead of one phi arg per ssa def. This unlocks some cases for redundant branch opts. In some pathological cases we may see extremely long phi arg lists. Will keep an eye out for that and perhaps modify the strategy if we end up with hundreds or thousands of phi args. Addresses an issue noted in dotnet#48115.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsRevise SSA to add one phi arg per pred instead of one phi arg per ssa def. This unlocks some cases for redundant branch opts. In some pathological cases we may see extremely long phi arg lists. Will keep an eye out for that and perhaps modify the strategy if we end up with hundreds or thousands of phi args. Addresses an issue noted in #48115.
|
|
@jakobbotsch PTAL Expect modest number of small diffs. Local TP run showed slight slowdown, but my local MSVC is also a bit out of date, so let's see what CI measures. |
Can you give an example of when this would be different? I would expect that multiple ssa defs reaching through the same pred would end up merged via a PHI def. I guess the same def can reach via multiple preds, and will now get multiple PhiArgs? |
|
TP Impact is as high as 0.25%, maybe a bit more than we should pay given the diffs.
Yes, the same ssa def can reach from multiple preds, eg int x = 2;
if (p && q) {
x = 3;
}
... = x; // phi here will have 3 phi args, two will bring in the value 2 and the other 3 |
| if (phiArg->GetSsaNum() == ssaNum) | ||
| { | ||
| // We already have this (pred, ssaNum) phiArg | ||
| return; |
There was a problem hiding this comment.
Does this situation happen in practice? Seems odd we'd add the same phi arg twice.
There was a problem hiding this comment.
I was roughly copying what the previous code did. I'll find out if we ever hit this case.
There was a problem hiding this comment.
Yes, this can happen.
AddPhiArgsToSuccessors will enumerate all the successors of a block via GetAllSuccs, and for those successors that are try entries, will also add phi args to handlers reachable from the try (since we assume that execution of that try might throw an exception while the def from block is the current def).
But the GetAllSuccs enumerator has similar logic for enumerating handlers as successors of a block with a try successor, so we end up redundantly trying to add PhiArgs.
Seems like this is fixable, but best done as its own zero diff change.
Guess it is just the cost of somewhat larger PHIs. Why this impacts ASP.NET more though I have no clue. I did a quick screen and across SPMI no PHI has more than 50 PHI ARGs. So it's not pathological cases at least. We can keep the PHI_ARGs ordered by gtPredBB->bbNum during ssa building and mitigate some of the search costs, but it seems painful to maintain any ordering here in general. Another option would be to go back to one PHI_ARG per value, but make it possible for let each PHI_ARG to contain a block list, or the union of a block list and a single block. Only RBO and RangeCheck look at this information (and I wonder how RangeCheck gets it right with current bits... seems like it might miss out on killing off assertions if it doesn't examine all the relevant preds). |
|
Maybe what I'll do is merge this and then work on a follow-on change to remove the need for searching and improve TP. |
ghost
locked as resolved and limited conversation to collaborators
Revise SSA to add one phi arg per pred instead of one phi arg per ssa def. This unlocks some cases for redundant branch opts.
In some pathological cases we may see extremely long phi arg lists. Will keep an eye out for that and perhaps modify the strategy if we end up with hundreds or thousands of phi args.
Addresses an issue noted in #48115.