Use rangecheck in assertprop#112766

EgorBo · 2025-02-20T23:01:46Z

Contributes to #112765 (the sign-extension cast).

The PR borrows @jakobbotsch's idea to re-use rangecheck outside of its phase (#110352)

src/coreclr/jit/rangecheck.cpp

EgorBo · 2025-02-21T21:49:45Z

PTAL @jakobbotsch @AndyAyersMS @dotnet/jit-contrib Diffs

Had to fix a bug inside MergeEdgeAssertions around unsigned comparisons

AndyAyersMS

Changes LGTM.

Up at the top of the method seems like we could hoist the // First, check simple properties without assertions code up above the initial set of bail-out checks?

jakobbotsch · 2025-02-21T22:56:41Z

src/coreclr/jit/assertionprop.cpp

+            rng.LowerLimit().AddConstant(cns);
+            rng.UpperLimit().AddConstant(cns);


Return value needs to be checked?

Ah, didn't realize AddConstant doesn't do anything if cns limit overflows. Added!

jakobbotsch · 2025-02-21T22:57:02Z

src/coreclr/jit/assertionprop.cpp

+                if ((rng.LowerLimit().GetConstant() == 0))
+                {
+                    *isKnownNonNegative = true;
+                }


Does this check not need to account for overflow?

but how can X - CNS + CNS overflow?

(AddConstant checks for overflow before it)

I was worried that for a range like "[8..unknown]", that "unknown" could mean "some computation that overflowed" and hence that the lower limit was actually smaller than 8. But it's probably ok since the range here comes from assertions only.

jakobbotsch · 2025-02-21T22:58:34Z

src/coreclr/jit/assertionprop.cpp

+        else
+        {
+            Range rng = Range(Limit(Limit::keDependent));
+            RangeCheck::MergeEdgeAssertions(this, treeVN, ValueNumStore::NoVN, assertions, &rng, false);
+            Limit lowerBound = rng.LowerLimit();
+            if (lowerBound.IsConstant())
+            {
+                if (lowerBound.GetConstant() >= 0)
+                {
+                    *isKnownNonNegative = true;
+                }
+                if (lowerBound.GetConstant() > 0)
+                {
+                    *isKnownNonZero = true;
+                }
+            }
+        }


Does this part catch cases? Naively I would expect assertion prop to know to check for the "straightforward" assertions that range check is checking here already.

Yep, good chunk of diffs are caught by this

I wonder if we could get rid of some of the checks above then, I would expect there to be a good amount of duplication between range check's assertion handling and that assertion handling above.

I agree, I'll check what is missing in the above

jakobbotsch · 2025-02-21T23:00:09Z

src/coreclr/jit/assertionprop.cpp

+                    *isKnownNonZero     = true;
+                }
+            }
+        }


IIRC, you had a version where you tried to use range check directly on the IR node. It seems like it would catch more cases, because the logic here for additions should already be conceptually the same logic that range check has to handle additions.

That version had terrible TP impact and needed more touches (perhaps, its cache can be shared with rangecheck?), I might investigate it again in the future

Actually, I'll do it after this PR - diffs look nice (if TP regressions are mitigated)

jakobbotsch · 2025-02-22T15:30:54Z

src/coreclr/jit/assertionprop.cpp

+            if ((rng.LowerLimit().IsConstant() && !rng.LowerLimit().AddConstant(cns)) ||
+                (rng.UpperLimit().IsConstant() && !rng.UpperLimit().AddConstant(cns)))
+            {
+                // Add cns to both bounds if they are constants. Make sure the addition doesn't overflow.
+                return;


Suggested change

if ((rng.LowerLimit().IsConstant() && !rng.LowerLimit().AddConstant(cns)) ||

(rng.UpperLimit().IsConstant() && !rng.UpperLimit().AddConstant(cns)))

{

// Add cns to both bounds if they are constants. Make sure the addition doesn't overflow.

return;

if (!rng.LowerLimit().AddConstant(cns) || !rng.UpperLimit().AddConstant(cns))

{

// Add cns to both bounds. Make sure the addition doesn't overflow.

return;

Should be ok since there's the check for IsConstant below already?

Nope. This is too conservative - we're fine with some bounds to be Dependent (non-constant) when we e.g. only check Lower one. AddConstant returns false for them making the improvement super conservative

jakobbotsch

LGTM. Would be nice to find some way to unify what assertion prop and range check are trying to do since they are starting to converge to trying to do the same thing it seems.

This reverts commit 6022adf.

This reverts commit db681fb.

Use rangecheck in assertprop

98e809e

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 20, 2025

dotnet-policy-service bot assigned EgorBo Feb 20, 2025

MihuBot mentioned this pull request Feb 21, 2025

[JitDiff X64] [EgorBo] Use rangecheck in assertprop MihuBot/runtime-utils#1012

Open

EgorBo added 2 commits February 21, 2025 20:17

Fix CI

597e2ad

clean up

3010416

IDisposable reviewed Feb 21, 2025

View reviewed changes

src/coreclr/jit/rangecheck.cpp Outdated Show resolved Hide resolved

Clean up

c239189

EgorBo requested review from AndyAyersMS and jakobbotsch February 21, 2025 21:49

EgorBo marked this pull request as ready for review February 21, 2025 21:49

AndyAyersMS approved these changes Feb 21, 2025

View reviewed changes

jakobbotsch reviewed Feb 21, 2025

View reviewed changes

Check overflow

08338d5

jakobbotsch reviewed Feb 22, 2025

View reviewed changes

jakobbotsch approved these changes Feb 22, 2025

View reviewed changes

EgorBo merged commit 6022adf into dotnet:main Feb 22, 2025
109 of 112 checks passed

EgorBo deleted the use-rangecheck-assertprop branch February 22, 2025 17:52

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Feb 24, 2025

Revert "Use rangecheck in assertprop (dotnet#112766)"

52c63d1

This reverts commit 6022adf.

AndyAyersMS added a commit that referenced this pull request Feb 24, 2025

Revert "Use rangecheck in assertprop (#112766)" (#112872)

db681fb

This reverts commit 6022adf.

EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Feb 25, 2025

Reapply "Use rangecheck in assertprop (dotnet#112766)" (dotnet#112872)

76e6032

This reverts commit db681fb.

EgorBo added a commit that referenced this pull request Feb 26, 2025

Reapply "Use rangecheck in assertprop (#112766)" (#112872) (#112907)

f99e58b

kunalspathak mentioned this pull request Mar 7, 2025

Test failure: System.Security.Cryptography.Xml.Tests.XmlDsigExcC14NTransformTest.ExcC14NSpecExample5 #113049

Closed

github-actions bot locked and limited conversation to collaborators Mar 25, 2025

		rng.LowerLimit().AddConstant(cns);
		rng.UpperLimit().AddConstant(cns);

Comments

Conversation

EgorBo commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

EgorBo commented Feb 21, 2025

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EgorBo commented Feb 20, 2025 •

edited

Loading

EgorBo Feb 22, 2025 •

edited

Loading

jakobbotsch Feb 22, 2025 •

edited

Loading

EgorBo Feb 22, 2025 •

edited

Loading