JIT: fix bug in loop cloning with down-counting loops#126770
JIT: fix bug in loop cloning with down-counting loops#126770AndyAyersMS wants to merge 1 commit intodotnet:mainfrom
Conversation
We were not creating proper cloning conditions, so the fast path might execute in cases where it shouldn't. We need to always verify for down counting that the initial value is strictly less than the array length(s).
github-actions
bot
added
the
area-CodeGen-coreclr
|
@jakobbotsch PTAL |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Fixes an issue in CoreCLR JIT loop cloning where down-counting loops could generate overly-permissive fast-path conditions, allowing bounds-check removal when the initial index equals the array/span length.
Changes:
- Tighten loop cloning limit condition derivation for down-counting loops to require the initial iterator value be strictly less than the relevant length.
- Add new JIT regression tests covering down-counting loops that start at
Lengthfor both arrays and spans. - Add a dedicated test project for the new regression test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/loopcloning.cpp | Adjusts derived cloning condition operator so down-counting loops require init < length rather than allowing init == length. |
| src/tests/JIT/opt/Cloning/DownCounted.cs | Adds regression coverage for array and span down-counting loops that should throw on a[Length] unless cloning conditions are correct. |
| src/tests/JIT/opt/Cloning/DownCounted.csproj | Adds a minimal test project definition consistent with other tests in the directory. |
Comments suppressed due to low confidence (1)
src/tests/JIT/opt/Cloning/DownCounted.cs:73
- There are extra blank lines (including whitespace-only lines) at the end of the file. Please trim trailing whitespace / remove the extra blank lines to keep diffs clean and comply with typical repo formatting rules.
}
| bool hasProblem = ArrayProblem(a, 100); | ||
| Console.WriteLine($"failed, has problem={hasProblem}"); | ||
| } | ||
| catch (IndexOutOfRangeException e) | ||
| { | ||
| Console.WriteLine("passed"); | ||
| result = 100; | ||
| } |
There was a problem hiding this comment.
The caught IndexOutOfRangeException is assigned to e but never used. Consider removing the exception variable (or using _) to avoid unused-variable warnings and match other tests in this directory.
| bool hasProblem = SpanProblem(a, 100); | ||
| Console.WriteLine($"failed, has problem={hasProblem}"); | ||
| } | ||
| catch (IndexOutOfRangeException e) | ||
| { | ||
| Console.WriteLine("passed"); | ||
| result = 100; | ||
| } |
There was a problem hiding this comment.
The caught IndexOutOfRangeException is assigned to e but never used. Consider removing the exception variable (or using _) to avoid unused-variable warnings and match other tests in this directory.
We were not creating proper cloning conditions, so the fast path might execute in cases where it shouldn't.
We need to always verify for down counting that the initial value is strictly less than the array length(s).