Clean up in rangecheck#129144
Open
EgorBo wants to merge 3 commits into
Open
Conversation
Remove the now-redundant optIsTreeKnownIntValue helper and the ad-hoc constant-index/arrSize handling in OptimizeRangeCheck. The array size is now derived consistently through GetRangeFromAssertions, which also recovers the exact size for new T[cns].Length via TryGetNewArrSize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors CoreCLR JIT bounds-check range analysis in rangecheck.cpp by removing a now-redundant constant-index fast path and a helper (optIsTreeKnownIntValue) in favor of using conservative VNs plus the existing assertion-based range machinery.
Changes:
- Simplifies
RangeCheck::OptimizeRangeCheckby dropping the dedicated constant-index /arrSizecomputation path and relying onTryGetRange+BetweenBounds. - Extends assertion-based range recovery for
VNF_ARR_LENGTHto tighten to an exact length fornew T[cns].LengthviaTryGetNewArrSize. - Removes
Compiler::optIsTreeKnownIntValuedeclaration and definition.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/rangecheck.cpp | Refactors bounds-check removal to rely on VN + assertion-based range info; enhances VNF_ARR_LENGTH assertion-range inference. |
| src/coreclr/jit/compiler.h | Removes the optIsTreeKnownIntValue declaration. |
| src/coreclr/jit/assertionprop.cpp | Removes the optIsTreeKnownIntValue implementation. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/rangecheck.cpp:806
GetRangeFromAssertionsWorkerreturns early for GC VN types (varTypeIsGC(vnType)), soVNF_StrFastAllocate(which represents a string ref VN) is effectively unreachable here. Leaving it in the int-range switch adds dead code and can mislead readers into thinkingStrFastAllocateis an int-producing VN.
case VNF_GT:
case VNF_GT_UN:
case VNF_GE:
case VNF_GE_UN:
case VNF_LT:
case VNF_LT_UN:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Minor cleanup in
rangecheck.cpp:GetRangeWorkerfor Array's length but didn't useDoesOverflowthat must be called forGetRangeWorker. Instead, I just call the VN-basedGetRangeFromAssertions(that plays nicely with overflows) and it seems to be enough, a few minor regression, a minor TP improvement.Note
This PR was generated with the assistance of GitHub Copilot.
Diffs (from an incorrect call to
GetRangeWorkerwithoutDoesOverflow)