More cleanups in rangecheck/assertionprop#129325
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR simplifies RyuJIT range-check handling by removing an unused “no range checks” configuration path and deleting the GTF_CHK_INDEX_INBND flag-based, deferred bounds-check removal logic in assertion propagation, replacing it with direct removal that preserves operand side effects.
Changes:
- Remove
FEATURE_ENABLE_NO_RANGE_CHECKS/JitNoRngChksconfig plumbing. - Remove
GTF_CHK_INDEX_INBNDand theGT_COMMAspecial-case path used to defer redundantGT_BOUNDS_CHECKremoval. - Remove the
optRemoveStandaloneRangeCheckwrapper and update bounds-check removal in assertion prop to directly replace the node with a side-effect-preservingNOTHING/COMMA.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/optimizer.cpp | Removes optRemoveStandaloneRangeCheck wrapper (standalone removal now handled differently). |
| src/coreclr/jit/jitconfigvalues.h | Deletes JitNoRngChks config entry (previously gated by a feature define). |
| src/coreclr/jit/gentree.h | Removes GTF_CHK_INDEX_INBND flag definition. |
| src/coreclr/jit/compiler.h | Removes declarations tied to deleted helpers (optRemoveStandaloneRangeCheck, optAssertionProp_Comma). |
| src/coreclr/jit/assertionprop.cpp | Removes deferred/comma-based bounds-check elimination and directly rewrites redundant GT_BOUNDS_CHECK to side-effect-only execution. |
| src/coreclr/inc/switches.h | Deletes the dormant FEATURE_ENABLE_NO_RANGE_CHECKS toggle comment/define. |
|
FYI I've seen people interested in enabling |
We've significantly improved the range check analysis over the last few years, so the need for elided checks should be much less relevant today. I'd suggest introducing helper calls that remove the checks and using them where the profiler points out a problem |
Mirror the hack in optRemoveRangeCheck: extract only GTF_ASG from the length and full side effects from the index, deliberately dropping the length load's null-check exception to avoid size regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
A small clean up PR with a few diffs Unfortunately, I had to keep (we have it today in PTAL @jakobbotsch @dotnet/jit-contrib |
* Remove `GTF_CHK_INDEX_INBND` and unconditionally always remove `GT_BOUNDS_CHECK` in assertprop when we can * Remove `FEATURE_ENABLE_NO_RANGE_CHECKS` and `DOTNET_JitNoRngChks`, these were never enabled by default and I don't think they have any value. * Correctness improvement: we no longer use this [potential bug](https://github.com/dotnet/runtime/blob/d71e0a994980fc47a1efb4ce2c207947e7efedca/src/coreclr/jit/optimizer.cpp#L5545-L5553) and extract all side-effects. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GTF_CHK_INDEX_INBNDand unconditionally always removeGT_BOUNDS_CHECKin assertprop when we canFEATURE_ENABLE_NO_RANGE_CHECKSandDOTNET_JitNoRngChks, these were never enabled by default and I don't think they have any value.