JIT: Document why the EH successor set is seemingly too large#94634
JIT: Document why the EH successor set is seemingly too large#94634jakobbotsch wants to merge 1 commit intodotnet:mainfrom
Conversation
I spent some time today figuring out why we have these additional (and seemingly unnecessary) successors when iterating EH successors, and found these two reasons.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsI spent some time today figuring out why we have these additional (and seemingly unnecessary) successors when iterating EH successors, and found these two reasons.
|
| // * It simplifies PHI arguments in handlers. In particular, when control is | ||
| // transferred to a handler, the reaching defs of live-in locals may come | ||
| // from predecessors of the try block if an exception was thrown before a def | ||
| // in the try. Without considering these as preds, we would have odd phi args | ||
| // (referring to blocks that aren't considered to be predecessors), or we | ||
| // would need to insert extra PHI definitions in the try block that the | ||
| // handler could refer to. |
There was a problem hiding this comment.
To be completely honest I think this alternative way of modelling things (adding "degenerate" phis at the entry try block) would be slightly more elegant.
There was a problem hiding this comment.
Interesting.
Along those lines we could require the first (real) statement of the try be some sort of fake def -- GT_ENTERTRY? It would mess up some other things like empty block detection and try removal, but I expect we can fix those things.
That in turn might make it easier to allow trys with arbitrary or multiple entry points, since (perhaps) we'd no longer have to guarantee that the lexical try entry dominated all the blocks in the try. Each entry would just need one of these special statements...
There was a problem hiding this comment.
Actually I don't think we need the degenerate phis... it is clearly not an uncommon scenario for a reaching def to be from some other block than the pred. So what I call out to be "odd" here is not actually odd.
As it turns out we already process the try heads specially, so I don't think my second reason described here exists. And it seems simple to generalize it to handle your case, we just need to generalize the following check to not be restricted to the head try block:
runtime/src/coreclr/jit/ssabuilder.cpp
Line 1247 in 2454fe6
So that only leaves the dominator tree computation, as far as I can tell... seems like it would be more reasonable to handle it specially there instead of overapproximating the EH successor/predecessor sets for everyone. I'll probably give this a try and see if I hit any other snags.
|
Superseded by #94672 |
2 participants
I spent some time today figuring out why we have these additional (and seemingly unnecessary) successors when iterating EH successors, and found these two reasons.