JIT: enable inlining methods with EH #112998
Conversation
Enable inlining of method with EH. Inlinee EH clauses are integrated into the root method EH table at the appropriate point (mid-table if the call site is in an EH region; at table end otherwise). Contributes to dotnet#108900.
ghost
added
the
area-CodeGen-coreclr
|
SPMI is not going to be helpful here. PMI may not be either, as callees with EH may not look like attractive candidates w/o PGO. No special work done on profitability, we just use whatever rules the JIT has already. Also expecting (though perhaps not in our tests) the usual slew of surprises as certain methods no longer show up in stack traces. |
But technically, inliner should resist a bit more for such methods? I think it's fine for this PR not to care about it |
|
Seems like the x86 crossgen issues are that we have managed methods with both EH and unmanaged calling conventions and we try and inline these and get confused by the call argument reversal. For now I'll just block inlining of managed methods with unmanaged conventions. |
|
Found a couple places where we rely on |
|
Various folks have reminded me that the EH clause reporting for catches is token based, and this will need to be generalized somehow to handle cross-module inlines. The assumption today is that the class token in the clause is from the same module as the root method. For runtime dependent types we resolve these tokens into code via For now we can perhaps restrict inlines with EH to be same module. |
|
Also looks like we need to update |
|
Hard to say what fraction of the failures are catch-type related, but likely quite a few. So am going to restrict to just try-finally for now. Also x86 still not happy. |
|
For x86, we need to remove the shadow SP slots var if we manage to remove all EH. Seems like what we ought to do is defer allocating this until we're done changing EH regions, then determine if we need this and the max nesting level. Going to spin that off as a separate change as it leads to some diffs on its own. |
|
Split off some of the enabling changes here: #113023. |
Don't try an inline managed methods with unmanaged calling conventions. This mainly copes with x86 where unmanaged calling conventions use reversed arg order, but I've disabled it in general. No diffs as these methods seem to always include EH. Remove uses of `compXcptnsCount` as this goes stale whenever we clone or remove EH, or (eventually) inline methods with EH. Instead, rely on `compHndBBtabCount`. Defer allocating x86's shadow SP var and area until later in jitting, so this reflects any changes in EH table structure. In particular we often are able to eliminate EH in part or all together and this saves a low-offset allocation and so leads to some nice code size savings on x86. Also on x86 remove the runtime-dependent catch class case from the computation for keeping this alive, as we now transform such into runtime lookups in filters (that may well keep this alive).
|
Down to 300-ish failures Libraries: X86 (widespread) R2R |
|
For the first assert. Before funclet splitting we have and afterwards we've broken up T1: Likely fallout from using Here we have But EH0 comes from an inlinee, so these are not mutual-protect regions. |
Once we can inline methods with EH, IL ranges are no longer a reliable indicator of a mutual-protect try regions. Instead, after importation, we can rely on mutual-protect trys having the same start and end blocks. Also update other case where we were using `info.compXcptnsCount` in morph to decide if we needed a frame pointer. This lets us simplify the logic around frame pointers and EH (though I still think we're making up our minds too early). Contributes to dotnet#108900.
|
Latest batch of issues x86: R2R: as above Arm64 Linux: widespread crypto failures |
|
@jkotas @davidwrighton The jit is triggering an assert in crossgen2 inside I suspect there's a missing check somewhere but not sure where it should be or what form it should take. This comes up in the R2R version resilience tests. |
|
Seems like maybe these stubs are not safely inlinable. Perhaps they have some residual state that does not get reset? The jit doesn't seem to have a way to detect that an inlinee is one of these stubs, it's only told if the root method is one (via jit flags). Perhaps it makes sense to mark these as noinline for now. |
|
@mihu-bot |
|
I think this is good to go, but am going to hold off merging until after the Preview3 snap. |
|
@AndyAyersMS let me take a look at the dump. This is a case on unhandled exception where we reraise the exception when there are no more managed frames on the stack that would handle it and we want to give a native caller a chance to catch that unhandled exception. There is a special handling in the |
| return; | ||
| } | ||
|
|
||
| // Do not inline pinvoke stubs with EH. |
There was a problem hiding this comment.
Should this comment describe why we are doing this and/or should we get an issue opened to fix this? The problem looked like a bug in R2R compiler to me.
There was a problem hiding this comment.
I will open an issue.
A few more details above: #112998 (comment)
|
|
/azp run runtime-coreclr libraries-pgo |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@AndyAyersMS, @jkotas I have found the culprit. It was introduced by my #112666, the added check at this line https://github.com/dotnet/runtime/pull/112666/files#diff-7769667aa99181e4e0ee0226f639f9a3aad3cd351fe15e883e765cda13e63764R8607 |
|
@janvorli let me know when you've fixed the issue. I'll hold off merging this until then. |
|
I was trying to repro the issue locally, but so far I was not successful. However, the culprit I have mentioned should not cause the problem we are seeing, because the While there are 24 distinct So I am starting to think that the issue has nothing to do with @AndyAyersMS's change, but it is rather a rare intermittent issue that just kicked in in this PR. |
|
|
|
Actually, in the crash dump, the thread abort was issued by other thread than the one running the test. I haven't noticed that before.
Ah no, there are two similar looking lambdas and I have mistakenly seen them as the same. |
|
After some offline discussion with @janvorli the issue above seems hard to repro and may well be pre-existing. So I'm going to merge this and see if the problem pops up in subsequent testing. |
Enable inlining of method with EH. Inlinee EH clauses are integrated into the root method EH table at the appropriate point (mid-table if the call site is in an EH region; at table end otherwise).
Contributes to #108900.