JIT: fix loop cloning logic for jagged arrays - #134153
AndyAyersMS wants to merge 2 commits into
Conversation
Fixes dotnet#133757. To safely reason about bounds checks for the inner arrays of jagged arrays we must prove those inner arrays cannot be re-assigned. Otherwise there is no up-front check we can make that will ensure the inner array accesses remain in bounds. Add an analysis pass that fires when a multidimensional array is seen during cloning that tries to prove that the inner array values are also loop invariants; if not, then the inner array bounds checks are not considered as part of the cloning criteria. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🔵 Needs a closer look
The synchronization bailout lacks a deterministic regression test.
Pull request overview
This PR fixes stale jagged-array bounds assumptions during loop cloning by detecting unsafe inner-array reference changes.
Changes:
- Adds cached loop-effect analysis for array-reference mutations and synchronization.
- Preserves cloning for safe accesses.
- Adds regression coverage for row replacement scenarios.
File summaries
| File | Reviewed changes |
|---|---|
src/tests/JIT/Regression_ro_2/Runtime_133757.cs |
Adds jagged-array regression and preservation tests. |
src/coreclr/jit/loopcloning.cpp |
Implements mutation and synchronization checks for cloning. |
src/coreclr/jit/compiler.h |
Adds cached loop-analysis state and declarations. |
Review details
Suppressed comments (1)
src/coreclr/jit/loopcloning.cpp:2589
- This new synchronization bailout is not covered by the regression file: the added cases exercise direct/aliased/reference stores and scalar or independent reference-element stores, but no volatile, memory-barrier, or atomic update of the row slot. Add a deterministic case that replaces the jagged row through one of these synchronization paths and verifies that the subsequent iteration still throws; otherwise this safety gate can regress without a test failure.
if (node->OperIs(GT_MEMORYBARRIER) || (node->OperIsIndir() && node->AsIndir()->IsVolatile()))
{
return WALK_ABORT;
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
|
@jakobbotsch this is the PR with the fairly complex analysis. It is likely a long-standing issue, but I still need to verify. We are trying to prove that the values of jagged-array sub-arrays cannot change during the loop, including from synchronized writes on other threads. We do this analysis once per cloning attempt; a failed result disqualifies all jagged sub-arrays from cloning criteria. Small number of diffs. |
|
@jakobbotsch ping |
| if (node->OperIs(GT_STOREIND) && !varTypeIsGC(node->TypeGet())) | ||
| { | ||
| return WALK_CONTINUE; |
| // Synchronization may make other-thread updates to array references observable. | ||
| // | ||
| if (node->OperIs(GT_MEMORYBARRIER) || (node->OperIsIndir() && node->AsIndir()->IsVolatile())) | ||
| { | ||
| return WALK_ABORT; |
| // Synchronization may make other-thread updates to array references observable. | ||
| // | ||
| if (node->OperIs(GT_MEMORYBARRIER) || (node->OperIsIndir() && node->AsIndir()->IsVolatile())) | ||
| { | ||
| return WALK_ABORT; | ||
| } |
There was a problem hiding this comment.
Is that sufficient? In practice we can observe the change without any explicit synchronization primitive, can't we?
jakobbotsch
left a comment
There was a problem hiding this comment.
LGTM as an improvement on the current situation, but in practice with the possibility of observing concurrent updates I am not so sure that we can really recover this optimization at all (without some kind of escape analysis).
I can try and see how often escape analysis can rescue this optimization, but aside from the inherent limitations of escape analysis, it will be limited to just the outermost array today. Given where we're headed with unsafe it seems like we must otherwise disable this optimization. I don't know what that means for perf, but (at least for uniform dimension cases) it tilts the scales in favor of multi-dimensonal arrays. So if we disable this, we should also finish the work on bringing those up to par. |
Fixes #133757.
Loop cloning can reuse stale bounds when a loop replaces a jagged array
row, even though the base-array and index locals remain invariant. Add a
cached loop-effects check before considering inner-array bounds checks
for cloning.
Reject potential array-reference mutations and synchronization while
keeping scalar element stores and independent one-dimensional accesses
eligible. The regression runs in the merged runner, using
AggressiveOptimizationon the loop helpers rather than process isolation.Validation
jit-format.Regression_ro_2with tiering enabledand disabled.
Codegen and performance
out of 987,559, net -764 bytes, with 908 matching missing contexts.
differences across 2,805,980 processed contexts in 12 collections.
InProd.Inner, original 700x700 workload with a non-inlined callee:+0.08% to +0.17% timing point estimates across three Release comparisons.
Paired BenchmarkDotNet runs were equivalent within a 1% threshold.
Measurements used one pinned logical CPU on a Hyper-V Xeon 8370C VM
with tiering disabled; they do not establish zero overhead.
Note
This PR description was generated with GitHub Copilot.