Skip to content

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 22, 2025

This PR does:

  1. Folds more various "implied relops" in optAssertionPropGlobal_RelOp with help of RangeCheck::TryGetRangeFromAssertions (it's not too expensive). For now, it only handles "Op1 relop IntCns". For that, I introduced EvalRelop that can answer questions about two ranges (e.g. range1 is >= range2, etc)
  2. Improved MergeEdgeAssertions a bit:
    • if VN is actually ArrayLen, then for bounds-check NO_THROW assertions we can also check if our vn equals lenVN - this gives us a hint that lenVN is >= indexVN (in case of constant)
    • for X >= CNS we now produce "[CNS...INT32_MAX]" instead of "[CNS...UNKNOWN]" - this helps DoesOverflow to be less conservative for full constant ranges

One note that RangeCheck::TryGetRangeFromAssertions is more or less cheap - it relies only on assertions. There is also RangeCheck::TryGetRange that is SSA-based - it finds a lot more places to optimize, with it, I see -250k diffs. Unfortunately, we have to pay with up to +1% TP for them. I want to invest some time in improving TP in RangeCheck to see if I can get 80% of th e diffs for a small price

Example of an improved pattern:

void Test(Span<int> src)
{
    src[5] = 42; // creates an assertion
    Consume(src.Slice(4)); // fold checks here based on the assertion ^
}

Diffs

@EgorBo
Copy link
Member Author

EgorBo commented Feb 23, 2025

@MihuBot

@EgorBo EgorBo marked this pull request as ready for review March 2, 2025 19:19
Copilot AI review requested due to automatic review settings March 2, 2025 19:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 2, 2025

/azp run runtime-coreclr libraries-pgo, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Mar 3, 2025

/azp run runtime-coreclr libraries-pgo, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Mar 3, 2025

PTAL @jakobbotsch @dotnet/jit-contrib diffs

I ended up removing SSA-based for now for two reasons:

  1. it's too TP heavy
  2. it has a small issue in DoesOverflow here (it should be properly handled)

I'll try again separately, SSA-based one increases diffs significantly (up to -250k for win-x64)

Fuzzlyn's failures repro on Main too, although, looks like some of them are my fault (previous PRs) - I'm looking now

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the description to describe what this does now?

if (tree->OperIsCmpCompare() &&
// We can also use a more powerful SSA-based TryGetRange, but it's too expensive to call for every relop.
// Hence, the following checks are driven by TP/CQ balance:
op1->TypeIs(TYP_INT) && op2->IsIntCnsFitsInI32() && !op2->IsIntegralConst(0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use op2's VN instead relying on it being a literal constant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I just assumed that we always propagate constants by value before global assertprop so while we generally should rely on VNs, for constants it's more or less fine 🙂 but in the sake of consistency I guess I can

Copy link
Member Author

@EgorBo EgorBo Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndyAyersMS I've addressed your feedback in 877112c and updated the issue description

@EgorBo EgorBo changed the title Use SSA-based ComputeRange in assertprop Use TryGetRangeFromAssertions to fold more relops in global assert prop Mar 5, 2025
@EgorBo EgorBo merged commit 4729bf9 into dotnet:main Mar 5, 2025
113 checks passed
@EgorBo EgorBo deleted the rngchk-2 branch March 5, 2025 10:42
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

2 participants