Fix type initialization ordering under R2R #84275
Conversation
ghost
added
the
Remove pIsHoistable from second method - it returns the call that can be checked directly
|
Some code size measurements: R2R - ReadyToRun fix Minor variations appear to be due to the repo changing day-to-day, so the takeaway is simply that applying the optimization to the old fix gets that regression back and the new fix+optimization has very little impact. #N/A occurred when the job didn't find the .mch file.
|
|
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
|
Azure Pipelines successfully started running 2 pipeline(s). |
markples
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsExpand the fix in #80485 to cover helpers including common ones for R2R as well as some generic helpers for JIT. This would cause regressions (100ks code size in diffs) if done alone, so it also includes checks on whether a helper may trigger a cctor and whether it is beforefieldinit. The precise ordering of the fix is only required for a non-beforefieldinit ctor. This recovers the (small) losses from #80485 and mitigates this fix. As before, the primary diff is moving type initialization into (for correctness) or out of (the mitigation optimization) a JIT tree. These leads to secondary effects as values may need to be preserved around the type initialization code. The end result of the fix+mitigations is very little code size change, but viewing the diffs suggests possible future code improvements. However, the impact of any of these should be established first.
Fixes #84007
|
|
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
test results: runtime
outerloop
extra-platforms has tons of failures. It's very hard to tell what's going on, but other jobs that I've looked at seem to have similar failures. |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@AndyAyersMS Could you please review this one? |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM with one commenting nit.
| CORINFO_FIELD_INFO* pFieldInfo, | ||
| var_types lclTyp) | ||
| var_types lclTyp, | ||
| /* OUT */ bool* pIsHoistable) |
There was a problem hiding this comment.
Add a header comment? I know we still have many without these comments but I try to add one for the methods I'm changing.
There was a problem hiding this comment.
I attempted one.. learning a bit about those types in the process.
(the merge conflict was between code that I moved and main removed)
2 participants
Expand the fix in #80485 to cover helpers including common ones for R2R as well as some generic helpers for JIT.
This would cause regressions (100ks code size in diffs) if done alone, so it also includes checks on whether a helper may trigger a cctor and whether it is beforefieldinit. The precise ordering of the fix is only required for a non-beforefieldinit ctor. This recovers the (small) losses from #80485 and mitigates this fix.
As before, the primary diff is moving type initialization into (for correctness) or out of (the mitigation optimization) a JIT tree. These leads to secondary effects as values may need to be preserved around the type initialization code.
The end result of the fix+mitigations is very little code size change, but viewing the diffs suggests possible future code improvements. However, the impact of any of these should be established first.
Fixes #84007