RangeCheck: Don't create invalid ranges

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!

[EgorBo]

Fixes a bug @amanasifkhalid hit in #113709

It is possible to create ranges where their lower limit is greater than the upper in MergeEdgeAssertions. E.g.

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

Some of our range operations (RangeOps::Merge specifically) do invalid operations on such ranges, so instead of fixing such places, I propose we don't create them in the first place (give up on them).

Normally, this should not cause issues as such array accesses likely will never be accessible anyway, but we now use range analysis in assertprop

[dotnet-policy-service]

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

[EgorBo]

/azp list

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[EgorBo]

@dotnet/jit-contrib @amanasifkhalid PTAL

In theory, invalid ranges are fine to have e.g.:

if (i > 10 && i < 5)
{
arr[i] = 0; // never reachable in fact.
}

and we still may extract useful info from that, but looks like many existing Range operations do not expect them so it's safer to just give up on them. They used to be not a problem when we used Range only for bounds checks, but we now use it for actual transformations in AssertProp as well.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[am11]

Were we expecting regressions? e.g. linux arm64 seeing +1.22% in

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Line 258 in 7ed5ede

privatestaticvoidInsertionSort(Span<T>keys,Comparison<T>comparer)

[tannergooding]

In theory, invalid ranges are fine to have e.g.:

Are we currently dead code eliminating these cases?

[amanasifkhalid]

@EgorBo have you had a chance to look at the insertion sort regression?

[EgorBo]

Sorry for the delayed response.

Are we currently dead code eliminating these cases?

RangeCheck uses a powerful and slow "Assertions + SSA-based analysis" mechanism and we don't use the same thing to fold unreachable branches. We re-use a limited form of it in the Global Assertion prop, but we don't use SSA-based analysis there because it's too slow (adds >1% TP regression), we only use assertions.

Were we expecting regressions?

I've looked at those and while they look unfortunate (just a few functions), JIT had no right to optimize them based on what it analyzed: https://www.diffchecker.com/53WlCbZL/
e.g.

Tightening pRange: [<Dependent, -1>] with assertedRange: [<0, -1>] into [<0, -1>]

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ArraySortHelper.cs

Lines 1039 to 1057 in 0eb2b0a

	privatestaticvoidInsertionSort(Span<TKey>keys,Span<TValue>values)
	{
	for(inti=0;i<keys.Length-1;i++)
	{
	TKeyt=keys[i+1];
	TValuetValue=values[i+1];
	intj=i;
	while(j>=0&&(t==null||LessThan(reft,refkeys[j])))
	{
	keys[j+1]=keys[j];
	values[j+1]=values[j];
	j--;
	}
	keys[j+1]=t!;
	values[j+1]=tValue;
	}
	}

This is just too complex flow for current jit's BCE to properly analyze it.

Given that this PR unlocks @amanasifkhalid PR and fixes access violation in #116571 I think we should just accept it.

I might still allow these functions to report "invalid" ranges like this, but at the moment I think various RangeOps:: simply don't expect them and produce invalid results.

Also, for e.g. [10..-10] assertprop might assume that the value is never negative since the lower bound is > 0.

[amanasifkhalid]

LGTM. CI is in pretty rough shape, so we might want to wait for the next run before merging. Thanks!