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

Handle more scenarios for loop cloning #67930

Merged
merged 7 commits into from
Apr 26, 2022

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Apr 12, 2022

  • Increasing loops that are incremented by more than 1 like "i += 2"

TypeHashingAlgorithm.cs

for (int i = startIndex; i < src.Length; i += 2)
{
_hash1 = (_hash1 + _rotl(_hash1, 5)) ^ src[i];
if ((i + 1) < src.Length)
_hash2 = (_hash2 + _rotl(_hash2, 5)) ^ src[i + 1];
}

  • Decreasing loops like "i--", "i -= 2", etc.

internal virtual int LastIndexOf(T[] array, T value, int startIndex, int count)
{
int endIndex = startIndex - count + 1;
for (int i = startIndex; i >= endIndex; i--)
{
if (Equals(array[i], value))
{
return i;
}
}
return -1;
}
#endif
}

PriorityQueue's Heapify method.

for (int index = lastParentWithChildren; index >= 0; --index)
{
MoveDownDefaultComparer(nodes[index], index);
}

Contributes to #65342

@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 Apr 12, 2022
@ghost ghost assigned kunalspathak Apr 12, 2022
@ghost
Copy link

ghost commented Apr 12, 2022

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

Issue Details
  • Increasing loops that are incremented by more than 1 like "i += 2"
  • Decreasing loops like "i--", "i -= 2", etc.
Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

Failures are related to #67878

@kunalspathak kunalspathak marked this pull request as ready for review April 13, 2022 22:27
@kunalspathak
Copy link
Member Author

kunalspathak commented Apr 13, 2022

@dotnet/jit-contrib , @BruceForstall

@BruceForstall
Copy link
Member

You need to worry about increment/decrement overflowing a 32-bit integer and wrapping. E.g.:

public static int test_up_big(int[] a, int s)
    {
        int r = 0;
        int i;
        for (i = 1; i < s; i += 2147483647)
        {
            r += a[i];
        }
        return r;
    }

with your code will clone, but generate an access violation, not System.IndexOutOfRangeException.

In the case of for ( ; i < x; i += c), c needs to be strictly less than int.MaxValue - (Array.MaxLength - 1) + 1, which is 0x7fffffff - 0x7fffffc7 + 2 = 0x3a = 58. The idea being that if i was the maximum allowed legal array index of 0x7fffffc6 and you added 0x3a, the result is negative (-2,147,483,648).

Similar applies to down counted loops.

@kunalspathak
Copy link
Member Author

public static int test_up_big(int[] a, int s)
{
int r = 0;
int i;
for (i = 1; i < s; i += 2147483647)
{
r += a[i];
}
return r;
}

Thanks for pointing that out. Let me try to fix that.

@kunalspathak
Copy link
Member Author

@BruceForstall - I tried measuring some of the benchmarks that showed up in asmdiffs and they all show improvements.

Faster base/diff Base Median (ns) Diff Median (ns) Modality
BilinearTest.Interpol_Scalar 1.23 12591.21 10245.69
BilinearTest.Interpol_Vector 1.08 27857.16 25908.72
BenchmarksGame.FannkuchRedux_5.RunBench(n: 10, expectedSum: 38) 1.06 16428450.00 15492476.47
BenchmarksGame.ReverseComplement_1.RunBench 1.13 1021397.94 903558.01
ByteMark.BenchEmFloatClass 1.05 771413700.00 734018150.00
ByteMark.BenchEmFloat 1.03 3737632200.00 3641513150.00
System.Collections.Tests.Perf_PriorityQueue<String, String>.HeapSort(Size: 100) 10.80 898710.07 83248.11
System.Collections.Tests.Perf_PriorityQueue<String, String>.HeapSort(Size: 10) 10.44 39025.89 3739.26
System.Collections.Tests.Perf_PriorityQueue<String, String>.HeapSort(Size: 1000) 10.34 14949560.00 1445691.48
System.Collections.Tests.Perf_PriorityQueue<Int32, Int32>.HeapSort(Size: 10) 1.04 203.82 195.96
Benchstone.BenchI.HeapSort.Test 1.03 294538.92 287004.51
Slower diff/base Base Median (ns) Diff Median (ns) Modality
System.Collections.Tests.Perf_PriorityQueue<Guid, Guid>.HeapSort(Size: 100) 1.02 9744.88 9900.55
System.Collections.Tests.Perf_PriorityQueue<Guid, Guid>.HeapSort(Size: 10) 1.01 396.27 401.45

Here is an example asmdiff from Heapify:

image

@kunalspathak
Copy link
Member Author

ping.

@BruceForstall BruceForstall self-requested a review April 22, 2022 21:55
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/compiler.hpp Outdated Show resolved Hide resolved
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/loopcloning.cpp Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 22, 2022
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM.

I think you should run a jitstress job before merging.

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

kunalspathak commented May 5, 2022

Improvements on windows/x64: dotnet/perf-autofiling-issues#5072
Improvements on windows/arm64: dotnet/perf-autofiling-issues#5145
Improvements on windows/x64: dotnet/perf-autofiling-issues#5114 (Reverse complement)
Improvements on Ubuntu/x64: dotnet/perf-autofiling-issues#5092
Improvements on Windows/x86: dotnet/perf-autofiling-issues#5028

@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2022
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