Improve constant folding for some frozen objects #86318

EgorBo · 2023-05-16T12:08:59Z

Just an exercise to improve constant folding in VN around frozen objects. As a demonstration:

void Foo() => Bar("TruE");

void Bar(string str)
{
    if (bool.TryParse(str, out bool value))
        Console.WriteLine(value);
}

New codegen:

; Method Program:Foo():this
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+20H], rax
       mov      byte  ptr [rsp+30H], 0
       mov      dword ptr [rsp+30H], 1
       movzx    rcx, byte  ptr [rsp+30H]
       call     [System.Console:WriteLine(bool)]
       nop      
       add      rsp, 56
       ret      
; Total bytes of code: 41

old codegen was terrible

; Method Program:Foo():this
G_M52879_IG01:  ;; offset=0000H
       push     rsi
       sub      rsp, 112
       vzeroupper 
       vxorps   xmm4, xmm4, xmm4
       vmovdqa  xmmword ptr [rsp+20H], xmm4
       vmovdqa  xmmword ptr [rsp+30H], xmm4
       vmovdqa  xmmword ptr [rsp+40H], xmm4
       vmovdqa  xmmword ptr [rsp+50H], xmm4
       vmovdqa  xmmword ptr [rsp+60H], xmm4
						;; size=42 bbWeight=1 PerfScore 12.58

G_M52879_IG02:  ;; offset=002AH
       mov      rdx, 0x21000209280      ; 'TruE'
       add      rdx, 12
       mov      bword ptr [rsp+60H], rdx
       mov      dword ptr [rsp+68H], 4
       lea      rdx, bword ptr [rsp+60H]
       cmp      dword ptr [rdx+08H], 4
       jne      SHORT G_M52879_IG05
						;; size=38 bbWeight=1 PerfScore 7.00

G_M52879_IG03:  ;; offset=0050H
       mov      rdx, bword ptr [rdx]
       mov      rcx, 0x20002000200020
       or       rcx, qword ptr [rdx]
       mov      rdx, 0x65007500720074
       cmp      rcx, rdx
       jne      SHORT G_M52879_IG05
						;; size=31 bbWeight=0.25 PerfScore 1.69

G_M52879_IG04:  ;; offset=006FH
       mov      ecx, 1
       jmp      G_M52879_IG24
						;; size=10 bbWeight=0.50 PerfScore 1.12

G_M52879_IG05:  ;; offset=0079H
       lea      rcx, bword ptr [rsp+60H]
       mov      rdx, bword ptr [rcx]
       cmp      dword ptr [rcx+08H], 5
       jne      SHORT G_M52879_IG08
						;; size=14 bbWeight=0.50 PerfScore 3.25

G_M52879_IG06:  ;; offset=0087H
       mov      rcx, 0x20002000200020
       or       rcx, qword ptr [rdx]
       mov      rax, 0x73006C00610066
       xor      rcx, rax
       mov      edx, dword ptr [rdx+06H]
       or       edx, 0x200020
       xor      edx, 0x650073
       or       rdx, rcx
       jne      SHORT G_M52879_IG08
						;; size=46 bbWeight=0.25 PerfScore 1.88

G_M52879_IG07:  ;; offset=00B5H
       xor      ecx, ecx
       jmp      G_M52879_IG24
						;; size=7 bbWeight=0.50 PerfScore 1.12

G_M52879_IG08:  ;; offset=00BCH
       vmovdqu  xmm0, xmmword ptr [rsp+60H]
       vmovdqu  xmmword ptr [rsp+50H], xmm0
						;; size=12 bbWeight=0.50 PerfScore 2.00

G_M52879_IG09:  ;; offset=00C8H
       mov      esi, dword ptr [rsp+58H]
       cmp      dword ptr [rsp+58H], 5
       jl       G_M52879_IG22
						;; size=15 bbWeight=0.50 PerfScore 2.00

G_M52879_IG10:  ;; offset=00D7H
       vmovdqu  xmm0, xmmword ptr [rsp+50H]
       vmovdqu  xmmword ptr [rsp+20H], xmm0
						;; size=12 bbWeight=0.50 PerfScore 2.00

G_M52879_IG11:  ;; offset=00E3H
       lea      rdx, [rsp+20H]
       lea      rcx, [rsp+50H]
       call     [System.Boolean:TrimWhiteSpaceAndNull(System.ReadOnlySpan`1[ushort]):System.ReadOnlySpan`1[ushort]]
       cmp      dword ptr [rsp+58H], esi
       je       G_M52879_IG22
						;; size=26 bbWeight=0.50 PerfScore 3.50

G_M52879_IG12:  ;; offset=00FDH
       vmovdqu  xmm0, xmmword ptr [rsp+50H]
       vmovdqu  xmmword ptr [rsp+40H], xmm0
						;; size=12 bbWeight=0.50 PerfScore 2.00

G_M52879_IG13:  ;; offset=0109H
       lea      rcx, bword ptr [rsp+40H]
       cmp      dword ptr [rcx+08H], 4
       jne      SHORT G_M52879_IG16
						;; size=11 bbWeight=0.50 PerfScore 2.25

G_M52879_IG14:  ;; offset=0114H
       mov      rcx, bword ptr [rcx]
       mov      rax, 0x20002000200020
       or       rax, qword ptr [rcx]
       mov      rcx, 0x65007500720074
       cmp      rax, rcx
       jne      SHORT G_M52879_IG16
						;; size=31 bbWeight=0.25 PerfScore 1.69

G_M52879_IG15:  ;; offset=0133H
       mov      ecx, 1
       mov      eax, 1
       jmp      SHORT G_M52879_IG23
						;; size=12 bbWeight=0.50 PerfScore 1.25

G_M52879_IG16:  ;; offset=013FH
       xor      ecx, ecx
						;; size=2 bbWeight=0.50 PerfScore 0.12

G_M52879_IG17:  ;; offset=0141H
       vmovdqu  xmm0, xmmword ptr [rsp+50H]
       vmovdqu  xmmword ptr [rsp+30H], xmm0
						;; size=12 bbWeight=0.50 PerfScore 2.00

G_M52879_IG18:  ;; offset=014DH
       lea      rax, bword ptr [rsp+30H]
       mov      rdx, bword ptr [rax]
       cmp      dword ptr [rax+08H], 5
       jne      SHORT G_M52879_IG20
						;; size=14 bbWeight=0.50 PerfScore 3.25

G_M52879_IG19:  ;; offset=015BH
       mov      rax, 0x20002000200020
       or       rax, qword ptr [rdx]
       mov      r8, 0x73006C00610066
       xor      rax, r8
       mov      edx, dword ptr [rdx+06H]
       or       edx, 0x200020
       xor      edx, 0x650073
       or       rax, rdx
       sete     al
       movzx    rax, al
       jmp      SHORT G_M52879_IG21
						;; size=52 bbWeight=0.25 PerfScore 2.44

G_M52879_IG20:  ;; offset=018FH
       xor      eax, eax
						;; size=2 bbWeight=0.25 PerfScore 0.06

G_M52879_IG21:  ;; offset=0191H
       jmp      SHORT G_M52879_IG23
						;; size=2 bbWeight=0.50 PerfScore 1.00

G_M52879_IG22:  ;; offset=0193H
       xor      ecx, ecx
       xor      eax, eax
						;; size=4 bbWeight=0.50 PerfScore 0.25

G_M52879_IG23:  ;; offset=0197H
       test     eax, eax
       je       SHORT G_M52879_IG25
						;; size=4 bbWeight=0.50 PerfScore 0.62

G_M52879_IG24:  ;; offset=019BH
       call     [System.Console:WriteLine(bool)]
						;; size=6 bbWeight=0.50 PerfScore 1.50

G_M52879_IG25:  ;; offset=01A1H
       nop      
						;; size=1 bbWeight=1 PerfScore 0.25

G_M52879_IG26:  ;; offset=01A2H
       add      rsp, 112
       pop      rsi
       ret      
						;; size=6 bbWeight=1 PerfScore 1.75
; Total bytes of code: 424

ghost · 2023-05-16T12:11:39Z

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

Issue Details

Just an excercise to improve constant folding in VN around frozen objects (can be useful for simialr cases). As a demonstration:

void Foo() => Bar("TruE");

void Bar(string str)
{
    if (bool.TryParse(str, out bool value))
        Console.WriteLine(value);
}

New codegen:

; Method Program:Foo():this
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+20H], rax
       mov      byte  ptr [rsp+30H], 0
       mov      dword ptr [rsp+30H], 1
       movzx    rcx, byte  ptr [rsp+30H]
       call     [System.Console:WriteLine(bool)]
       nop      
       add      rsp, 56
       ret      
; Total bytes of code: 41

(old codegen was terrible)

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

ShreyasJejurkar · 2023-05-16T13:17:34Z

This is great improvement.

And also why old codegen is this much bad, provided the value passed to function is constant string and not even dynamic one! 🙄😕

EgorBo · 2023-05-16T13:34:42Z

This is great improvement.

And also why old codegen is this much bad, provided the value passed to function is constant string and not even dynamic one! 🙄😕

It's not bad, it's just too big (a lot of stuff inlined), boolean parsing is done via two OridnalIgnoreCase comparisons, e.g.:

internal static bool IsFalseStringIgnoreCase(ReadOnlySpan<char> value)
{
    return value.Equals("False", StringComparison.OrdinalIgnoreCase);
}

emits:

; Method Program:IsFalseStringIgnoreCase(System.ReadOnlySpan`1[ushort]):bool
G_M35886_IG01:  ;; offset=0000H
						;; size=0 bbWeight=1 PerfScore 0.00

G_M35886_IG02:  ;; offset=0000H
       mov      rax, bword ptr [rcx]
       cmp      dword ptr [rcx+08H], 5
       jne      SHORT G_M35886_IG04
						;; size=9 bbWeight=1 PerfScore 6.00

G_M35886_IG03:  ;; offset=0009H
       mov      rdx, 0x20002000200020
       or       rdx, qword ptr [rax]
       mov      rcx, 0x73006C00610066
       xor      rdx, rcx
       mov      eax, dword ptr [rax+06H]
       or       eax, 0x200020
       xor      eax, 0x650073
       or       rax, rdx
       sete     al
       movzx    rax, al
       jmp      SHORT G_M35886_IG05
						;; size=50 bbWeight=0.25 PerfScore 2.44

G_M35886_IG04:  ;; offset=003BH
       xor      eax, eax
						;; size=2 bbWeight=0.25 PerfScore 0.06

G_M35886_IG05:  ;; offset=003DH
       ret      
						;; size=1 bbWeight=1 PerfScore 1.00
; Total bytes of code: 62

EgorBo · 2023-05-16T13:48:14Z

@jakobbotsch @SingleAccretion PTAL since you're familiar with this pattern (when we accumulate a final byte offset out of ADD(ADD(ADD... trees) @dotnet/jit-contrib

There are jit-diff improvements, but SPMI is empty because of missing contexts (this opt opens new oportunities to fold frozen objects but we need a JIT-VM api call)

EgorBo · 2023-05-16T13:49:44Z

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

@@ -267,6 +273,7 @@ public static bool TryParse(ReadOnlySpan<char> value, out bool result)

            return TryParseUncommon(value, out result);

+            [MethodImpl(MethodImplOptions.NoInlining)]
            static bool TryParseUncommon(ReadOnlySpan<char> value, out bool result)


TryParseUncommon used to be inlineable in some cases (e.g. without pgo data)

EgorBo · 2023-05-16T13:50:07Z

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

+        {
+            if (!TryParse(value, out bool result))
+            {
+                ThrowHelper.ThrowFormatException_BadBoolean(value);


Replaced throw with a throw helper to make it inlineable (at least for constant input)

src/coreclr/jit/valuenum.cpp

EgorBo · 2023-05-16T14:52:25Z

I wanted to give it a try in Rust with the same code to see how it looks, is a significant difference. Not sure if it's something related to our limitations that we can't have optimization like this or if it's how it is.

here is the code (C# vs Rust) https://godbolt.org/z/ddnYh1Mc5

Judging by your snippet, Rust doesn't support ignore case parsing so the codegen you see is responsible to return some parsing error for you

ShreyasJejurkar · 2023-05-16T14:55:17Z

Yeah, my mistake, I did not look correctly, that it gave an error! Sometimes compiler explorer windows are so confusing! 😅😅

ShreyasJejurkar · 2023-05-16T15:39:04Z

@EgorBo I have one doubt, in the below example, can't we generate the same codegen for WithParse method just like we generated for WithoutTryParse given the fact that the input to them are constant string and would be available at compile time itself; Can't JIT see that and further optimize it so that it can we cam avoid those unwanted jump instructions!

https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgMwAEOZAwmQN5FFlMXnAD2bANmQOpQIALNgFcEAFQBOATwAKECSgCmACgCUdRsy2YA7GXZcAdHIUqARAgnDFZ1QG5NTAL6Oyr0vo7c+gk0rUahFpaBtwSiijCnAgOQcFMUABmZMqhhpKy8v4WVjZwZCIIZOGR0aqqrlr0cfHaeiVRMZXMLjXBumSJEJxKsVqtTkA==

EgorBo · 2023-05-16T15:42:48Z

@EgorBo I have one doubt, in the below example, can't we generate the same codegen for WithParse method just like we generated for WithoutTryParse given the fact that the input to them are constant string and would be available at compile time itself; Can't JIT see that and further optimize it so that it can we cam avoid those unwanted jump instructions!

https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgMwAEOZAwmQN5FFlMXnAD2bANmQOpQIALNgFcEAFQBOATwAKECSgCmACgCUdRsy2YA7GXZcAdHIUqARAgnDFZ1QG5NTAL6Oyr0vo7c+gk0rUahFpaBtwSiijCnAgOQcFMUABmZMqhhpKy8v4WVjZwZCIIZOGR0aqqrlr0cfHaeiVRMZXMLjXBumSJEJxKsVqtTkA==

these are two different methods which inline differently. Also, this PR fixes constant folding

ShreyasJejurkar · 2023-05-16T15:48:12Z

@EgorBo I have one doubt, in the below example, can't we generate the same codegen for WithParse method just like we generated for WithoutTryParse given the fact that the input to them are constant string and would be available at compile time itself; Can't JIT see that and further optimize it so that it can we cam avoid those unwanted jump instructions!
https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgMwAEOZAwmQN5FFlMXnAD2bANmQOpQIALNgFcEAFQBOATwAKECSgCmACgCUdRsy2YA7GXZcAdHIUqARAgnDFZ1QG5NTAL6Oyr0vo7c+gk0rUahFpaBtwSiijCnAgOQcFMUABmZMqhhpKy8v4WVjZwZCIIZOGR0aqqrlr0cfHaeiVRMZXMLjXBumSJEJxKsVqtTkA==

these are two different methods which inline differently. Also, this PR fixes constant folding

Ohh, I see. I thought it would be the same as those constant strings are valid boolean values, so we can optimize the codegen for TryParse, or maybe Short-Circuting the call to Parse directly from that method once we found that specific input string. But it's ok, there might be some reason of not doing that.

Also, this PR fixes constant folding

I wish, there was an easier way to see codegen of the given program against the PR on browser itself😅😅

SingleAccretion

Jit changes LGTM.

EgorBo · 2023-05-16T18:17:27Z

cc @dotnet/jit-contrib for a green sign off

EgorBo · 2023-05-17T14:52:12Z

@dotnet/jit-contrib @jakobbotsch Ping, this PR also fixes a cross-compile bug @yowl hit in the NativeAOT-LLVM branch

jakobbotsch · 2023-05-17T16:42:36Z

src/coreclr/jit/valuenum.cpp

        {
-            *pObj       = vnStore->ConstantObjHandle(funcApp.m_args[0]);
-            *byteOffset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[1]);


I'm surprised we haven't hit this assert with this previous code:

runtime/src/coreclr/jit/valuenum.h

Lines 1080 to 1099 in bda1f6c

T val1 = reinterpret_cast<T*>(c->m_defs)[offset];

T val2 = SafeGetConstantValue<T>(c, offset);

// Detect if there is a mismatch between the VN storage type and explicitly

// passed-in type T.

bool mismatch = false;

if (varTypeIsFloating(c->m_typ))

{

mismatch = (memcmp(&val1, &val2, sizeof(val1)) != 0);

}

else

{

mismatch = (val1 != val2);

}

if (mismatch)

{

assert(

!"Called ConstantValue<T>(vn), but type(T) != type(vn); Use CoercedConstantValue instead.");

}

I would expect it to hit for 64-bit to 32-bit crossgen2 compilations. It seems to indicate that this code is never hit for crossgen2 32-bit compilations? Is that expected?

I would expect it to hit for 64-bit to 32-bit crossgen2 compilations.

That's correct - it will be hit if you replay a 32 bit collection with a cross-targeting Jit today. Evidently, we just don't have people (or machines) doing that.

crossgen doesn't have frozen objects so this path indeed has never been executed, but we could see it on NativeAOT, but I'm not sure we cover the cross compilation case on CI for it

yowl · 2023-05-17T18:01:42Z

Thanks!

EgorBo added 2 commits May 16, 2023 13:47

Make jit fold bool.parse("true")

eb1a0fb

Fix typo

546f545

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

ghost assigned EgorBo May 16, 2023

EgorBo requested a review from jakobbotsch May 16, 2023 13:48

EgorBo commented May 16, 2023

View reviewed changes

SingleAccretion reviewed May 16, 2023

View reviewed changes

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

Use target_ssize_t

ca93999

SingleAccretion reviewed May 16, 2023

View reviewed changes

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

Update valuenum.cpp

71439a2

SingleAccretion approved these changes May 16, 2023

View reviewed changes

build-analysis bot mentioned this pull request May 16, 2023

DataContractSerializerTests.DCS_MyPersonSurrogate_Stress failing in CI #35066

Open

jakobbotsch reviewed May 17, 2023

View reviewed changes

jakobbotsch approved these changes May 17, 2023

View reviewed changes

EgorBo merged commit 83f71b5 into dotnet:main May 17, 2023

EgorBo deleted the fold-bool-parse branch May 17, 2023 18:00

JulieLeeMSFT mentioned this pull request Jun 9, 2023

What's new in .NET 8 Preview 5 [WIP] dotnet/core#8436

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve constant folding for some frozen objects #86318

Improve constant folding for some frozen objects #86318

EgorBo commented May 16, 2023 •

edited

Loading

ghost commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

EgorBo commented May 16, 2023

EgorBo commented May 16, 2023 •

edited

Loading

EgorBo May 16, 2023

EgorBo May 16, 2023

EgorBo commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

EgorBo commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

SingleAccretion left a comment

EgorBo commented May 16, 2023

EgorBo commented May 17, 2023

jakobbotsch May 17, 2023 •

edited

Loading

SingleAccretion May 17, 2023

EgorBo May 17, 2023

yowl commented May 17, 2023

	T val1 = reinterpret_cast<T*>(c->m_defs)[offset];
	T val2 = SafeGetConstantValue<T>(c, offset);

	// Detect if there is a mismatch between the VN storage type and explicitly
	// passed-in type T.
	bool mismatch = false;
	if (varTypeIsFloating(c->m_typ))
	{
	mismatch = (memcmp(&val1, &val2, sizeof(val1)) != 0);
	}
	else
	{
	mismatch = (val1 != val2);
	}

	if (mismatch)
	{
	assert(
	!"Called ConstantValue<T>(vn), but type(T) != type(vn); Use CoercedConstantValue instead.");
	}

Improve constant folding for some frozen objects #86318

Improve constant folding for some frozen objects #86318

Conversation

EgorBo commented May 16, 2023 • edited Loading

ghost commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

EgorBo commented May 16, 2023

EgorBo commented May 16, 2023 • edited Loading

EgorBo May 16, 2023

Choose a reason for hiding this comment

EgorBo May 16, 2023

Choose a reason for hiding this comment

EgorBo commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

EgorBo commented May 16, 2023

ShreyasJejurkar commented May 16, 2023

SingleAccretion left a comment

Choose a reason for hiding this comment

EgorBo commented May 16, 2023

EgorBo commented May 17, 2023

jakobbotsch May 17, 2023 • edited Loading

Choose a reason for hiding this comment

SingleAccretion May 17, 2023

Choose a reason for hiding this comment

EgorBo May 17, 2023

Choose a reason for hiding this comment

yowl commented May 17, 2023

EgorBo commented May 16, 2023 •

edited

Loading

EgorBo commented May 16, 2023 •

edited

Loading

jakobbotsch May 17, 2023 •

edited

Loading