Slightly improve string unrolling for length 5 and 6 #77398

EgorBo · 2022-10-24T19:18:24Z

Use 32bit load for the 2nd indirect load for length 5 and 6, it produces slightly better codegen, e.g.:

bool IsFalse(string s) => s.Equals("false", StringComparison.OrdinalIgnoreCase);
// same for Spans, StartsWith, etc.

Codegen diff:

; Assembly listing for method Program:IsFalse(System.String):bool:this
G_M56292_IG01:              
G_M56292_IG02:              
       837A0805             cmp      dword ptr [rdx+08H], 5
       7541                 jne      SHORT G_M56292_IG04
G_M56292_IG03:              
       48B82000200020002000 mov      rax, 0x20002000200020
       480B420C             or       rax, qword ptr [rdx+0CH]
       48B9660061006C007300 mov      rcx, 0x73006C00610066
       4833C1               xor      rax, rcx
-      48B92000200020002000 mov      rcx, 0x20002000200020
-      480B4A0E             or       rcx, qword ptr [rdx+0EH]
-      48BA61006C0073006500 mov      rdx, 0x650073006C0061
-      4833D1               xor      rdx, rcx
+      8B5212               mov      edx, dword ptr [rdx+12H]
+      81CA20002000         or       edx, 0x200020
+      81F273006500         xor      edx, 0x650073
       480BC2               or       rax, rdx
       0F94C0               sete     al
       0FB6C0               movzx    rax, al
       EB02                 jmp      SHORT G_M56292_IG05
G_M56292_IG04:              
       33C0                 xor      eax, eax
G_M56292_IG05:              
       C3                   ret      
-; Total bytes of code 74
+; Total bytes of code 62

Btw, it'd be nice if we could get rid of this in favor of ^ but it will regress mono.

ghost · 2022-10-24T19:18:39Z

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

Issue Details

Use 32bit load for the 2nd indirect load for length 5 and 6, it produces slightly better codegen, e.g.:

bool IsFalse(string s) => s.Equals("false", StringComparison.OrdinalIgnoreCase);

Codegen diff:

; Assembly listing for method Program:IsFalse(System.String):bool:this
G_M56292_IG01:              
G_M56292_IG02:              
       837A0805             cmp      dword ptr [rdx+08H], 5
       7541                 jne      SHORT G_M56292_IG04
G_M56292_IG03:              
       48B82000200020002000 mov      rax, 0x20002000200020
       480B420C             or       rax, qword ptr [rdx+0CH]
       48B9660061006C007300 mov      rcx, 0x73006C00610066
       4833C1               xor      rax, rcx
-      48B92000200020002000 mov      rcx, 0x20002000200020
-      480B4A0E             or       rcx, qword ptr [rdx+0EH]
-      48BA61006C0073006500 mov      rdx, 0x650073006C0061
-      4833D1               xor      rdx, rcx
+      8B5212               mov      edx, dword ptr [rdx+12H]
+      81CA20002000         or       edx, 0x200020
+      81F273006500         xor      edx, 0x650073
       480BC2               or       rax, rdx
       0F94C0               sete     al
       0FB6C0               movzx    rax, al
       EB02                 jmp      SHORT G_M56292_IG05
G_M56292_IG04:              
       33C0                 xor      eax, eax
G_M56292_IG05:              
       C3                   ret      
-; Total bytes of code 74
+; Total bytes of code 62

Btw, it'd be nice if we could get rid of this in favor of ^ but it will regress mono.

Author:	EgorBo
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2022-10-24T19:33:41Z

Although, the JIT version seems faster 🙂:

internal static bool IsFalseStringIgnoreCase_jit(ReadOnlySpan<char> value) => 
    value.Equals("false", StringComparison.OrdinalIgnoreCase);

internal static bool IsFalseStringIgnoreCase(ReadOnlySpan<char> value)
{
    // "fals" as a ulong, each char |'d with 0x0020 for case-insensitivity
    ulong fals_val = BitConverter.IsLittleEndian ? 0x73006C00610066ul : 0x660061006C0073ul;
    return value.Length == 5 &&
           (((MemoryMarshal.Read<ulong>(MemoryMarshal.AsBytes(value)) | 0x0020002000200020) == fals_val) &
            ((value[4] | 0x20) == 'e'));
}

codegen diff: https://www.diffchecker.com/AodEOtoa (jit one is on the left)

jkotas · 2022-10-24T19:58:21Z

Although, the JIT version seems faster

Change the CoreLib impl to this then?

EgorBo · 2022-10-24T20:07:02Z

Although, the JIT version seems faster

Change the CoreLib impl to this then?

Do we need to keep the hand-written version for mono? I feel like it should also be doable for Mono-LLVM without a lot of efforts - we can emit llvm.memcmp intrinsic here and LLVM will unroll it itself (not the mono-mini and mono-interp though)

stephentoub · 2022-10-24T21:02:18Z

Honest question, does this micro-optimization for bool.TryParse matter as much on mono? There are plenty of places where we use string.Equals(..., StringComparison.OrdinalIgnoreCase) everywhere and we don't try to special-case mono. I'd opt for simplicity, and if this particular optimization is important for mono, we should focus on imbuing it with the same optimization coreclr has.

EgorBo · 2022-10-24T21:03:40Z

I think we can drop it and restore if they/benchmarks scream at us 😄 I might try to implement the optimization for mono-llvm when I have time

jkotas · 2022-10-24T21:18:44Z

Honest question, does this micro-optimization for bool.TryParse matter as much on mono? There are plenty of places where we use string.Equals(..., StringComparison.OrdinalIgnoreCase) everywhere and we don't try to special-case mono

+1 . We have avoided introducing Mono-specific performance ifdefs for the most part and it served us well.

src/libraries/System.Private.CoreLib/src/System/Boolean.cs

jkotas · 2022-10-24T23:26:52Z

src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs

@@ -15,7 +15,7 @@ internal static bool GetBooleanConfig(string switchName, string envVariable, boo
            if (!AppContext.TryGetSwitch(switchName, out bool ret))
            {
                string? switchValue = Environment.GetEnvironmentVariable(envVariable);
-                ret = switchValue != null ? (bool.IsTrueStringIgnoreCase(switchValue) || switchValue.Equals("1")) : defaultValue;
+                ret = switchValue != null ? (switchValue.Equals("true", StringComparison.OrdinalIgnoreCase) || switchValue.Equals("1")) : defaultValue;


It is dangerous to call string.Equals(..., StringComparison.OrdinalIgnoreCase) here. It can call into globalization stack when the JIT optimizations does not kick in, and the globalization will call this method recursively to read its config.

ah, good point, I see that Ordinal.EqualsIgnoreCase has a fast path for ASCII but if the value in the config is not ASCII it might trigger globalization indeed, let me restore the previous version here

Good catch, I'm sad I was not smart enough to see it myself 😞

It would be also possible to fix this by changing OrdinalIgnoreCase to avoid initializing globalization.

OrdinalIgnoreCase will fallback to the slow path if one of the chars is not ASCII. It really only needs to fallback to the slow path if both chars are not ASCII.

@jkotas do you mind if I investigate it separately? I also wanted to look into vectorizing Ordinal.CompareStringIgnoreCase - it currently uses only SWAR version

Yes, that's fine

Mono tests are still failing though. You may need to fix this to make them pass.

Yep, changed CompareStringIgnoreCase

src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs

stephentoub · 2022-10-25T00:06:31Z

codegen diff: https://www.diffchecker.com/AodEOtoa (jit one is on the left)

Is it possible to hand-roll an implementation that results in as-good code, or is there something else missing in the JIT that would enable that? (I'm not suggesting we want to hand-roll this, but rather wondering if there's an additional gap to be addressed.)

benaadams · 2022-10-25T01:48:18Z

codegen diff: https://www.diffchecker.com/AodEOtoa (jit one is on the left)

Is it possible to hand-roll an implementation that results in as-good code, or is there something else missing in the JIT that would enable that? (I'm not suggesting we want to hand-roll this, but rather wondering if there's an additional gap to be addressed.)

Are two throws in asm for the non Jit version:

ArgumentOutOfRangeException in the MemoryMarshal.Read<ulong> (checking length)
OverflowException for the MemoryMarshal.AsBytes (computing new length checked(span.Length * Unsafe.SizeOf<T>()))

EgorBo · 2022-10-25T11:45:13Z

Are two throws in asm for the non Jit version:

ArgumentOutOfRangeException in the MemoryMarshal.Read<ulong> (checking length)

OverflowException for the MemoryMarshal.AsBytes (computing new length checked(span.Length * Unsafe.SizeOf<T>()))

Yep, OverflowException is a known case, if you look at the asm you will see that it's unreachable, it's just JIT is not able to remove it. We plan to eventually handle that, it's just low priority because it doesn't affect hot code. Same problem exists on arm64 with div.

ArgumentOutOfRangeException looks complicated to eliminate in JIT here 😞 but we can try.

Is it possible to hand-roll an implementation that results in as-good code,

this version is closer to what jit emits (modulo unaligned reads):

internal static bool IsFalseStringIgnoreCase(ReadOnlySpan<char> value)
{
    if (value.Length == 5)
    {
        ref char c = ref MemoryMarshal.GetReference(value);
        ulong fals_val = BitConverter.IsLittleEndian ? 0x73006C00610066ul : 0x660061006C0073ul;
        return (Unsafe.As<char, ulong>(ref c) | 0x0020002000200020 ^ fals_val | (value[4] | 0x20 ^ (uint)'e')) == 0;
    }
    return false;
}

EgorBo · 2022-10-25T15:33:51Z

jit-diffs: https://dev.azure.com/dnceng-public/public/_build/results?buildId=61917&view=ms.vss-build-web.run-extensions-tab

src/libraries/System.Private.CoreLib/src/System/Globalization/Ordinal.cs

Co-authored-by: Jan Kotas <[email protected]>

EgorBo · 2022-10-26T13:31:52Z

@jkotas @stephentoub it seems it still fails on Mono, it happens because mono has a slightly different behavior around static initialization order. This path https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs#L54 doesn't trigger CompareInfo..cctor() on CoreCLR (because it's unused from Boolean.cs), but it does on mono, likely just hoisted.

jkotas · 2022-10-26T15:38:01Z

CoreCLR will trigger the beforefieldinit static constructors exactly at the point of the first field reference. It is done intentionally to avoid unpredictable behavior and compat problems (#4346 (comment)).

I thought that Mono is on the same plan, but it is apparently not the case. Open a bug on this against Mono and add a workaround under MONO ifdef and a link to the bug?

EgorBo · 2022-10-26T20:16:32Z

Open a bug on this against Mono

#77513

src/libraries/System.Private.CoreLib/src/System/Boolean.cs

EgorBo · 2022-11-01T12:42:53Z

Improvements:

x64: [Perf] Linux/x64: 5 Improvements on 10/27/2022 2:21:25 PM perf-autofiling-issues#9336
arm64: [Perf] Windows/arm64: 2 Improvements on 10/27/2022 1:15:09 PM perf-autofiling-issues#9476

Regressions:

[Perf] Windows/arm64: 1 Regression on 10/27/2022 1:15:09 PM perf-autofiling-issues#9470

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 24, 2022

ghost assigned EgorBo Oct 24, 2022

Improve unrolling for len [5..6]

c3cd558

EgorBo force-pushed the improve-str-unroll branch from 927a264 to c3cd558 Compare October 24, 2022 19:19

Update importervectorization.cpp

294aab0

Use in Boolean.cs

922ce36

Clean up

9f52aed

jkotas reviewed Oct 24, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Boolean.cs Outdated Show resolved Hide resolved

jkotas reviewed Oct 24, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Boolean.cs Outdated Show resolved Hide resolved

EgorBo added 3 commits October 24, 2022 23:25

Address feedback

5db63b9

revert removed comment

f17692e

wrong one

13b7131

jkotas approved these changes Oct 24, 2022

View reviewed changes

Fix build

d821858

jkotas reviewed Oct 24, 2022

View reviewed changes

EgorBo added 2 commits October 25, 2022 01:40

Revert previous changes

26baa85

add comments

4bfab76

stephentoub reviewed Oct 25, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs Outdated Show resolved Hide resolved

Update AppContextConfigHelper.cs

e104f25

This was referenced Oct 25, 2022

Microsoft.DotNet.CoreSetup.Test.HostActivation tests failing in CI #75429

Closed

Long running test: ReadAsync_CancelPendingTask_ThrowsCancellationException and ReadAsync_CancelPendingValueTask_ThrowsCancellationException #72586

Closed

EgorBo added 2 commits October 25, 2022 16:23

Address feedback

8083cec

clean up

22ad3d5

jkotas reviewed Oct 25, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/Ordinal.cs Outdated Show resolved Hide resolved

jkotas reviewed Oct 25, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/Ordinal.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

e678b0a

Co-authored-by: Jan Kotas <[email protected]>

EgorBo mentioned this pull request Oct 26, 2022

Mono's behavior around .beforefieldinit #77513

Open

Address Jan's feedback

aa11c90

jakobbotsch reviewed Oct 27, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Boolean.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Oct 27, 2022

Tracking Nuget 429s dotnet/arcade#10885

Closed

2 tasks

jkotas approved these changes Oct 27, 2022

View reviewed changes

EgorBo merged commit 771eca7 into dotnet:main Oct 27, 2022

dakersnar mentioned this pull request Nov 15, 2022

[Perf] Alpine/x64: 2 Regressions on 11/14/2022 8:46:21 AM dotnet/perf-autofiling-issues#9728

Closed

ghost locked as resolved and limited conversation to collaborators Dec 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slightly improve string unrolling for length 5 and 6 #77398

Slightly improve string unrolling for length 5 and 6 #77398

EgorBo commented Oct 24, 2022 •

edited

Loading

ghost commented Oct 24, 2022

EgorBo commented Oct 24, 2022 •

edited

Loading

jkotas commented Oct 24, 2022

EgorBo commented Oct 24, 2022 •

edited

Loading

stephentoub commented Oct 24, 2022

EgorBo commented Oct 24, 2022 •

edited

Loading

jkotas commented Oct 24, 2022

jkotas Oct 24, 2022

EgorBo Oct 24, 2022

EgorBo Oct 24, 2022

jkotas Oct 25, 2022

EgorBo Oct 25, 2022

jkotas Oct 25, 2022

jkotas Oct 25, 2022

EgorBo Oct 25, 2022

stephentoub commented Oct 25, 2022

benaadams commented Oct 25, 2022

EgorBo commented Oct 25, 2022 •

edited

Loading

EgorBo commented Oct 25, 2022

EgorBo commented Oct 26, 2022 •

edited

Loading

jkotas commented Oct 26, 2022

EgorBo commented Oct 26, 2022

EgorBo commented Nov 1, 2022 •

edited by AndyAyersMS

Loading

Slightly improve string unrolling for length 5 and 6 #77398

Slightly improve string unrolling for length 5 and 6 #77398

Conversation

EgorBo commented Oct 24, 2022 • edited Loading

ghost commented Oct 24, 2022

EgorBo commented Oct 24, 2022 • edited Loading

jkotas commented Oct 24, 2022

EgorBo commented Oct 24, 2022 • edited Loading

stephentoub commented Oct 24, 2022

EgorBo commented Oct 24, 2022 • edited Loading

jkotas commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Oct 25, 2022

benaadams commented Oct 25, 2022

EgorBo commented Oct 25, 2022 • edited Loading

EgorBo commented Oct 25, 2022

EgorBo commented Oct 26, 2022 • edited Loading

jkotas commented Oct 26, 2022

EgorBo commented Oct 26, 2022

EgorBo commented Nov 1, 2022 • edited by AndyAyersMS Loading

EgorBo commented Oct 24, 2022 •

edited

Loading

EgorBo commented Oct 24, 2022 •

edited

Loading

EgorBo commented Oct 24, 2022 •

edited

Loading

EgorBo commented Oct 24, 2022 •

edited

Loading

EgorBo commented Oct 25, 2022 •

edited

Loading

EgorBo commented Oct 26, 2022 •

edited

Loading

EgorBo commented Nov 1, 2022 •

edited by AndyAyersMS

Loading