JIT: stop copying EH tab info into inlinee compiler #83622
Conversation
Having this info show up for inlinees just confuses things -- if the inlinee looks in this table and finds a block reference, it will be to a block that doesn't exist in the inlinee's flow graph. This has been tripping up profile synthesis, which wants to attribute some flow to EH entries; inlinees don't have any, but currently look like they do. There is one place in inlinee importation where we might ask about EH properties -- inline pinvoke checks. We're already careful to redirect this query to the call site block, so in the correspnonding lookup we must make sure to answer this via the root compiler's EH tab. If we ever decide to support inlining of methods with EH we'll have to reconcile all this differently.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsHaving this info show up for inlinees just confuses things -- if the inlinee looks in this table and finds a block reference, it will be to a block that doesn't exist in the inlinee's flow graph. This has been tripping up profile synthesis, which wants to attribute some flow to EH entries; inlinees don't have any, but currently look like they do. There is one place in inlinee importation where we might ask about EH properties -- inline pinvoke checks. We're already careful to redirect this query to the call site block, so in the correspnonding lookup we must make sure to answer this via the root compiler's EH tab. If we ever decide to support inlining of methods with EH we'll have to reconcile all this differently.
|
|
@EgorBo PTAL There is more we could (and perhaps should) do here, like:
|
|
Failures? Must have gotten automatically retried or something... |
|
No Diffs but SPMI sees some modest TP regressions. I can't imagine we call |
Using checked windows x64 jit on coreclr_tests (worst TP impact) there are 29783 methods out of 520968 that call one or both of these. checked
There are some sizeable regressions elsewhere (eg asp.net) so once I've looked into these pathological cases I'll look over there too. release632,931,084 total calls. MCs that make more than 1,000,000 calls shown below. Same set of suspects, checked just amplifies the problem.
|
|
aspnet similar data. 10K methods that call, 74M calls, avg 7K, top callers are more evenly distributed:
|
Could maybe be #78267 (comment) ? |
Looks like it is; here's a release profile:
|
|
Seems like we could revamp The extra cost here seems to come from successful searches that end up calling There is also likely some redundancy in these step block chains we could try and remove, eg if a region has two leaves with identical targets, or a region and its enclosing region share a leave target, etc. This would require more substantial reworking as we'd first want to build a graph of all the leave sources and targets and then compute the sets of step blocks needed to get everything to the right places. This could also be factored if we wanted to introduce some runtime state, eg if we have two deeply nested inner leaves with different outer targets they could at share the middle parts of the step block chain and just diverge at the last step based on the state. I suspect doing this would remove the need for That might reduce some of the impact here as we'd probably be creating fewer step blocks in some cases. But it's an even more radical reworking of leave handling. And not clear it's worth the effort overall as the vast majority of methods won't have huge numbers of leaves. But still looking the TP impact of this change on ASP.NET perhaps we have some motivation. |
Tried this and it was more complex than the above would make it seem, for a few reasons:
Still seems worth pursuing perhaps, but not as surgical as I'd hoped. Might be simpler in the short run to factor out some of the flow-inducing loop invariants |
|
Why do you need the changes to We don't inline functions containing EH, right? So the EH table can just remain empty? Even if we did, wouldn't the inlinee get its own EH table that would then get incorporated into the main function at the same time the blocks get incorporated (and renumbered)? |
We ask about some EH properties in the inlinee compiler -- the only one I know of for certain is whether a PInvoke call site is within a handler (since we might be inlining into a handler region of the root method, and there are/were some odd restrictions on how PInvokes should be handled in such cases). Currently we cope with this by copying the root compiler EH info into the inlinee compiler. Instead, we could explicitly redirect that query back to the root compiler instance. Assuming that's the only reason we copied the EH info to the inlinee then we could assert in the Seems worth a try. |
|
With #83610, the block numbers in the inlinee are in the same "namespace" as the inliner. Do any of these EH-related queries from the inlinee use this block number, and thus need to "translate" to the inliner block number namespace? |
Remembered a bit more about why I didn't try this -- the query for pinvokes is So if say we want Anyways let me add the asserts mentioned above and see where else we might be asking. |
I don't know for sure, but I suspect there aren't any that key off of Other structural properties of EH may not hold though. For instance, if an inlinee compiler managed to get a hold of a try region and thought it could enumerate all the blocks in the try by walking from begin to end while still in the process of inlining it would not see the inlinee blocks. But I don't think we do this sort of thing. |
|
Going to close this as this whole subject needs more thinking and isn't currently a problem. |
ghost
locked as resolved and limited conversation to collaborators
Having this info show up for inlinees just confuses things -- if the inlinee looks in this table and finds a block reference, it will be to a block that doesn't exist in the inlinee's flow graph.
This has been tripping up profile synthesis, which wants to attribute some flow to EH entries; inlinees don't have any, but currently look like they do.
There is one place in inlinee importation where we might ask about EH properties -- inline pinvoke checks. We're already careful to redirect this query to the call site block, so in the correspnonding lookup we must make sure to answer this via the root compiler's EH tab.
If we ever decide to support inlining of methods with EH we'll have to reconcile all this differently.