Skip to content

Remove mono specific SpanHelpers #78015

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.

Sign up for GitHub

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

Closed
BrzVlad wants to merge 12 commits into from
Closed

Remove mono specific SpanHelpers #78015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d946c3b
Revert "[Mono] Restore old code to solve the recent SpanHelpers regre…
BrzVlad Nov 1, 2022
3b73bb5
[System.Span] Optimize hot loops by simplifying offset computation
BrzVlad Nov 1, 2022
a48fb6f
[System.Span] Refactor hot loop code
BrzVlad Nov 1, 2022
fe06a72
[mono][interp] Replace compare + brfalse/brtrue with single condition…
BrzVlad Nov 1, 2022
ff36071
[mono][interp] Dump in/out links for bblocks during verbose logging
BrzVlad Nov 1, 2022
fb2bcde
[mono][interp] Improve detection of dead bblocks
BrzVlad Nov 1, 2022
5379908
[mono][interp] Reorder bblocks to facilitate propagation of values
BrzVlad Nov 3, 2022
c8edcde
[mono][interp] Don't optimize out bblocks that are tiering patchpoint…
BrzVlad Nov 15, 2022
cffdc02
[mono][interp] Make bblock reordering more conservative
BrzVlad Nov 15, 2022
5e8571f
[mono][interp] Add basic removal of unused defines
BrzVlad Nov 3, 2022
e4abb67
[mono][interp] Clear unused defines of local only vars
BrzVlad Nov 3, 2022
954483a
[mono][interp] Propagate target branches
BrzVlad Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[System.Span] Optimize hot loops by simplifying offset computation 
Before, for every single element, the address `address + offset + ct` had to be computed. In theory, a smart compiler would be able to reuse `address + offset` value and offset it only by a constant to obtain every single element. Do this explicitly to avoid reliance on advanced optimizations.
  • Loading branch information
@BrzVlad
BrzVlad committed Nov 18, 2022
commit 3b73bb57ca376c899a502686878b967a0b8db929
36 changes: 20 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,21 +364,22 @@ internal static unsafe int IndexOfNullByte(ref byte searchSpace)
{
lengthToExamine -= 8;

if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset))
ref byte current = ref Unsafe.AddByteOffset(ref searchSpace, offset);
if (uValue == current)
goto Found;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 1))
if (uValue == Unsafe.AddByteOffset(ref current, 1))
goto Found1;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 2))
if (uValue == Unsafe.AddByteOffset(ref current, 2))
goto Found2;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 3))
if (uValue == Unsafe.AddByteOffset(ref current, 3))
goto Found3;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 4))
if (uValue == Unsafe.AddByteOffset(ref current, 4))
goto Found4;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 5))
if (uValue == Unsafe.AddByteOffset(ref current, 5))
goto Found5;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 6))
if (uValue == Unsafe.AddByteOffset(ref current, 6))
goto Found6;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 7))
if (uValue == Unsafe.AddByteOffset(ref current, 7))
Copy link
Member

@stephentoub stephentoub Nov 8, 2022

Choose a reason for hiding this comment

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

Do these changes actually help? On coreclr at least it's not clear to me just from the diff that it's a net win.
https://www.diffchecker.com/du4Cevvb
Can you share throughput numbers?

Copy link
Member Author

@BrzVlad BrzVlad Nov 8, 2022

Choose a reason for hiding this comment

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

There shouldn't be any improvement on coreclr. I wouldn't be surprised if the generated code is more or less identical, since the JIT might make use of common subexpression elimination optimization. Also the code paths tweaked in this PR are for non vectorized cases, which are mostly not hit at all with coreclr.

The BCL change is intended for less optimized compilers to be able to generate good quality code.

Copy link
Member

@stephentoub stephentoub Nov 8, 2022

Choose a reason for hiding this comment

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

There shouldn't be any improvement on coreclr.

I'm more concerned it might regress. At least according to the diff I linked above, the new asm is a bit longer and appears to use a different addressing mode. I don't have a good sense for what that might mean in terms of throughput.

Also the code paths tweaked in this PR are for non vectorized cases, which are mostly not hit at all with coreclr.

Aren't they used for shorter inputs that don't have enough data to fill a vector?

Copy link
Member Author

@BrzVlad BrzVlad Nov 23, 2022

Choose a reason for hiding this comment

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

The local perf numbers on some benchmarks are unclear, I'm waiting for a full performance run.

goto Found7;

offset += 8;
Expand All @@ -388,13 +389,14 @@ internal static unsafe int IndexOfNullByte(ref byte searchSpace)
{
lengthToExamine -= 4;

if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset))
ref byte current = ref Unsafe.AddByteOffset(ref searchSpace, offset);
if (uValue == current)
goto Found;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 1))
if (uValue == Unsafe.AddByteOffset(ref current, 1))
goto Found1;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 2))
if (uValue == Unsafe.AddByteOffset(ref current, 2))
goto Found2;
if (uValue == Unsafe.AddByteOffset(ref searchSpace, offset + 3))
if (uValue == Unsafe.AddByteOffset(ref current, 3))
goto Found3;

offset += 4;
Expand Down Expand Up @@ -985,10 +987,12 @@ public static nuint CommonPrefixLength(ref byte first, ref byte second, nuint le

for (; (nint)i <= (nint)length - 4; i += 4)
{
if (Unsafe.Add(ref first, i + 0) != Unsafe.Add(ref second, i + 0)) return i + 0;
if (Unsafe.Add(ref first, i + 1) != Unsafe.Add(ref second, i + 1)) return i + 1;
if (Unsafe.Add(ref first, i + 2) != Unsafe.Add(ref second, i + 2)) return i + 2;
if (Unsafe.Add(ref first, i + 3) != Unsafe.Add(ref second, i + 3)) return i + 3;
ref byte currentFirst = ref Unsafe.Add(ref first, i);
ref byte currentSecond = ref Unsafe.Add(ref second, i);
if (currentFirst != currentSecond) return i + 0;
if (Unsafe.Add(ref currentFirst, 1) != Unsafe.Add(ref currentSecond, 1)) return i + 1;
if (Unsafe.Add(ref currentFirst, 2) != Unsafe.Add(ref currentSecond, 2)) return i + 2;
if (Unsafe.Add(ref currentFirst, 3) != Unsafe.Add(ref currentSecond, 3)) return i + 3;
}

return length;
Expand Down
Loading