Improve handling of VNF_Cast in GetRangeFromAssertionsWorker#128923
Draft
tannergooding wants to merge 1 commit into
Draft
Improve handling of VNF_Cast in GetRangeFromAssertionsWorker#128923tannergooding wants to merge 1 commit into
tannergooding wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts JIT range inference to better reason about VNF_Cast in RangeCheck::GetRangeFromAssertionsWorker, and updates value-number constant extraction to use a 64-bit intermediate when checking whether an integral VN constant fits in a requested target type.
Changes:
ValueNumStore::IsVNIntegralConstantnow coerces constants toint64_tbefore applyingFitsIn<T>, avoiding width-related truncation issues.GetRangeFromAssertionsWorkerrefines how it selects an initial range forVNF_Castby distinguishing widening vs. narrowing/same-size casts.GetRangeFromAssertionsWorkeradds anIsVNConstantfast-path for constants (but currently risks returning a non-constant range in a code path that is expected to produce constant ranges).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/valuenum.h | Uses a 64-bit coerced constant (int64_t) when probing whether an integral VN constant fits in a requested type. |
| src/coreclr/jit/rangecheck.cpp | Improves VNF_Cast range selection (widening vs. narrowing) and adjusts constant handling in GetRangeFromAssertionsWorker. |
Comment on lines
+713
to
+726
| // If it's a constant, it's already as tight as it can get. | ||
| int cns; | ||
| if (comp->vnStore->IsVNIntegralConstant(num, &cns)) | ||
| if (comp->vnStore->IsVNConstant(num)) | ||
| { | ||
| return Range(Limit(Limit::keConstant, cns)); | ||
| int cns; | ||
| if (comp->vnStore->IsVNIntegralConstant(num, &cns)) | ||
| { | ||
| return Range(Limit(Limit::keConstant, cns)); | ||
| } | ||
| else | ||
| { | ||
| // TODO: We could return `0, keUnknown` if the constant is known positive | ||
| // but this would require more handling in other places to take advantage of. | ||
| return Limit(Limit::keUnknown); | ||
| } |
tannergooding
force-pushed
the
better-castcheck
branch
from
94fca19 to
99ddd44
Compare
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.
This is a smaller change from #128906 that doesn't involve more complex handling around
TYP_LONG