Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SSA-based ComputeRange in assertprop #112824

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 22, 2025

This PR calls SSA-based RangeCheck::GetRange in Assertprop where it's benficial. It's not cheap so I've placed it in strategic places under TP-driven conditions.
Diffs.

What this PR does:

  1. Makes RangeCheck a singleton instance that is shared between assertprop and rangecheck (mainly, to save allocations for hash tables). All inner state is reset on each GetRange call anyway. Assertprop uses custom budgets (to improve TP).
  2. Removes DEBUG-only SSA-validation from rangecheck - it's quite verbose & messy, rangecheck used to be the last phase to use SSA, now we have more phases, so if we want some validation, it has to be something generic, not ad-hoc (which is also not-singleton friendly).
  3. m_pCurBndsChk field is replaced with ValueNum m_preferredBound because this is how it's used anyway (it's a hint for RangeCheck that m_preferredBound is more precise than a constant) - it's never used inside Assertprop uses.
  4. Global assertprop now responsible for marking blocks with BBF_MAY_HAVE_BOUNDS_CHECKS - this is 0 diff change with -0.1% TP improvement. BTW, currently rangecheck always analyzes even cold block - if we change that, we can get some nice TP wins.
  5. SSA-based GetRange() is called now from 2 places:
    • in optAssertionProp_RangeProperties (only if onlyIfCheap is false)
    • in optAssertionPropGlobal_RelOp for "x relop cns" to catch various "impled" cases (which RBO didn't catch)
  6. Slightly improved Ranges for relop-based assertions, e.g. signed X >= CNS now creates [CNS...INT_MAX] range instead of [CNS...UNKNOWN] - this helps DoesOverflow to be a bit less conservative.

The end goal is to be able to do things like this:

for (int i = 0; i < 1000; i++)
{
    Foo(i % 8); // i is never negative => mod is unsigned => i & 7
}

to be able to mark the mod as unsigned (since i never goes negative), so it's optimized into just i & 7. Same for other things where we might benefit from such knowledge - e.g. sign extension casts.

Far future TODOs

  1. RangeCheck should support 64-bit ranges, currently it's 32bit only

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 22, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@EgorBo
Copy link
Member Author

EgorBo commented Feb 23, 2025

@MihuBot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant