Fold "cns"[cns] for ROS<char> #78593

EgorBo · 2022-11-19T13:42:36Z

Closes #78359
e.g.:

int Test() => DoWork("D");

[MethodImpl(MethodImplOptions.AggressiveInlining)]
int DoWork(ReadOnlySpan<char> format)
{
    if (format.Length == 1)
    {
        switch (format[0] | 0x20)
        {
            case 'a': return 1;
            case 'b': return 2;
            case 'c': return 4;
            case 'd': return 8;
            case 'e': return 16;
            case 'f': return 32;
        }
    }
    throw new Exception();
}

Current codegen for Test:

; Method ConsoleApp8.C:Test():int:this
G_M000_IG01:                ;; offset=0000H
       56                   push     rsi
       4883EC20             sub      rsp, 32
G_M000_IG02:                ;; offset=0005H
       48B84023807D78020000 mov      rax, 0x2787D802340
       488B00               mov      rax, gword ptr [rax]
       4883C00C             add      rax, 12
       0FB700               movzx    rax, word  ptr [rax]
       83C820               or       eax, 32
       83C09F               add      eax, -97
       83F805               cmp      eax, 5
       7746                 ja       SHORT G_M000_IG10
       8BC0                 mov      eax, eax
       488D0D63000000       lea      rcx, [reloc @RWD00]
       8B0C81               mov      ecx, dword ptr [rcx+4*rax]
       488D15CEFFFFFF       lea      rdx, G_M000_IG02
       4803CA               add      rcx, rdx
       FFE1                 jmp      rcx
G_M000_IG03:                ;; offset=003CH
       B801000000           mov      eax, 1
       EB21                 jmp      SHORT G_M000_IG09
G_M000_IG04:                ;; offset=0043H
       B802000000           mov      eax, 2
       EB1A                 jmp      SHORT G_M000_IG09
G_M000_IG05:                ;; offset=004AH
       B804000000           mov      eax, 4
       EB13                 jmp      SHORT G_M000_IG09
G_M000_IG06:                ;; offset=0051H
       B808000000           mov      eax, 8
       EB0C                 jmp      SHORT G_M000_IG09
G_M000_IG07:                ;; offset=0058H
       B810000000           mov      eax, 16
       EB05                 jmp      SHORT G_M000_IG09
G_M000_IG08:                ;; offset=005FH
       B820000000           mov      eax, 32
G_M000_IG09:                ;; offset=0064H
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret      
G_M000_IG10:                ;; offset=006AH
       48B9582E82E0FA7F0000 mov      rcx, 0x7FFAE0822E58
       E84744AC5F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       488BCE               mov      rcx, rsi
       FF153BB10800         call     [System.Exception:.ctor():this]
       488BCE               mov      rcx, rsi
       E843509F5F           call     CORINFO_HELP_THROW
       CC                   int3     
RWD00  	dd	00000037h ; case G_M000_IG03
       	dd	0000003Eh ; case G_M000_IG04
       	dd	00000045h ; case G_M000_IG05
       	dd	0000004Ch ; case G_M000_IG06
       	dd	00000053h ; case G_M000_IG07
       	dd	0000005Ah ; case G_M000_IG08
; Total bytes of code: 142

New codegen for Test:

; Method ConsoleApp8.C:Test():int:this
       B808000000           mov      eax, 8
       C3                   ret      
; Total bytes of code: 6

ghost · 2022-11-19T13:42:44Z

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

Issue Details

Closes #78359
e.g.:

int Test() => DoWork("D");

[MethodImpl(MethodImplOptions.AggressiveInlining)]
int DoWork(ReadOnlySpan<char> format)
{
    if (format.Length == 1)
    {
        switch (format[0] | 0x20)
        {
            case 'a': return 1;
            case 'b': return 2;
            case 'c': return 4;
            case 'd': return 8;
            case 'e': return 16;
            case 'f': return 32;
        }
    }
    throw new Exception();
}

Current codegen for Test:

; Method ConsoleApp8.C:Test():int:this
G_M000_IG01:                ;; offset=0000H
       56                   push     rsi
       4883EC20             sub      rsp, 32
G_M000_IG02:                ;; offset=0005H
       48B84023807D78020000 mov      rax, 0x2787D802340
       488B00               mov      rax, gword ptr [rax]
       4883C00C             add      rax, 12
       0FB700               movzx    rax, word  ptr [rax]
       83C820               or       eax, 32
       83C09F               add      eax, -97
       83F805               cmp      eax, 5
       7746                 ja       SHORT G_M000_IG10
       8BC0                 mov      eax, eax
       488D0D63000000       lea      rcx, [reloc @RWD00]
       8B0C81               mov      ecx, dword ptr [rcx+4*rax]
       488D15CEFFFFFF       lea      rdx, G_M000_IG02
       4803CA               add      rcx, rdx
       FFE1                 jmp      rcx
G_M000_IG03:                ;; offset=003CH
       B801000000           mov      eax, 1
       EB21                 jmp      SHORT G_M000_IG09
G_M000_IG04:                ;; offset=0043H
       B802000000           mov      eax, 2
       EB1A                 jmp      SHORT G_M000_IG09
G_M000_IG05:                ;; offset=004AH
       B804000000           mov      eax, 4
       EB13                 jmp      SHORT G_M000_IG09
G_M000_IG06:                ;; offset=0051H
       B808000000           mov      eax, 8
       EB0C                 jmp      SHORT G_M000_IG09
G_M000_IG07:                ;; offset=0058H
       B810000000           mov      eax, 16
       EB05                 jmp      SHORT G_M000_IG09
G_M000_IG08:                ;; offset=005FH
       B820000000           mov      eax, 32
G_M000_IG09:                ;; offset=0064H
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret      
G_M000_IG10:                ;; offset=006AH
       48B9582E82E0FA7F0000 mov      rcx, 0x7FFAE0822E58
       E84744AC5F           call     CORINFO_HELP_NEWSFAST
       488BF0               mov      rsi, rax
       488BCE               mov      rcx, rsi
       FF153BB10800         call     [System.Exception:.ctor():this]
       488BCE               mov      rcx, rsi
       E843509F5F           call     CORINFO_HELP_THROW
       CC                   int3     
RWD00  	dd	00000037h ; case G_M000_IG03
       	dd	0000003Eh ; case G_M000_IG04
       	dd	00000045h ; case G_M000_IG05
       	dd	0000004Ch ; case G_M000_IG06
       	dd	00000053h ; case G_M000_IG07
       	dd	0000005Ah ; case G_M000_IG08
; Total bytes of code: 142

New codegen for Test:

; Method ConsoleApp8.C:Test():int:this
       B808000000           mov      eax, 8
       C3                   ret      
; Total bytes of code: 6

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

EgorBo · 2022-11-19T19:57:21Z

@SingleAccretion can you please take a look at the VN side when you have time, we discussed this one yesterday. For the issue with the side-effects I decided to just call gtUpdateStmtSideEffects for current statement and it worked.

SingleAccretion · 2022-11-19T20:01:17Z

src/coreclr/jit/valuenum.cpp

+{
+    ValueNum  addrVN = tree->gtGetOp1()->gtVNPair.GetLiberal();
+    VNFuncApp funcApp;
+    if (!varTypeIsShort(tree) || !vnStore->GetVNFunc(addrVN, &funcApp))


Why are we interested in TYP_SHORT (signed) trees, since I assume this is a TP heuristic?

This shouldn't have to recompute addrVN + GetVNFunc, since the caller already did that.

@SingleAccretion can you point me where exactly caller already did it? it seems like other paths also compute it

runtime/src/coreclr/jit/valuenum.cpp

Lines 8838 to 8853 in b406c17

else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic))

{

fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]);

offset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[2]);

// Note VNF_PtrToStatic statics are currently always "simple".

fgValueNumberFieldLoad(tree, /* baseAddr */ nullptr, fldSeq, offset);

}

else if (tree->OperIs(GT_IND) && fgValueNumberConstLoad(tree->AsIndir()))

{

// VN is assigned inside fgValueNumberConstLoad

}

else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToArrElem))

{

fgValueNumberArrayElemLoad(tree, &funcApp);

}

.

Changed to TYP_SHORT (yes, it was a TP fast-out path)

can you point me where exactly caller already did it? it seems like other paths also compute it

else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) computes it. It would be good to change this to compute the thing once like ASG numbering does.

do you mind if I leave it as is, I've just pushed a change to extract vnfunc only after TP-oriented checks. I tried to re-organize code to save these two existing lookups and didn't like the outcome, feel free to file a PR to clean it up

src/coreclr/jit/valuenum.cpp

Co-authored-by: SingleAccretion <[email protected]>

am11 · 2022-11-19T23:47:46Z

        switch (format[0] | 0x20)

Looks like with char.ToLowerInvariant() it emits an extra mov and a jmp.

+            switch (char.ToLowerInvariant(value[0]))
-            switch (value[0] | 0x20)

C.M2()
+    L0000: mov rcx, 0x207d38020d8
+    L000a: mov rcx, [rcx]
+    L000d: jmp 0x00007ff863530090
-    L0000: mov eax, 8
-    L0005: ret

and gets worse with value.ToLowerInvariant()[0]. Maybe importer can normalize them to | 0x20 equivalent for ASCII / A-Za-z values (since ToLowerInvariant() is more common than | 0x20).

EgorBo · 2022-11-20T00:31:40Z

@am11 good point! That jump is a tail call to TextInfo:ToLowerInvariant which is not inlined, I've pushed a change that fixes that - 369179d, e.g.:

char foo() => char.ToLowerInvariant('B');

Was:

; Method Prog:foo():ushort:this
       B942000000           mov      ecx, 66 ; 'B'
       FF25556B1500         tail.jmp [System.Globalization.TextInfo:ToLowerInvariant(ushort):ushort]
; Total bytes of code: 11

Now:

; Method Prog:foo():ushort:this
       B862000000           mov      eax, 98 ; 'b'
       C3                   ret      
; Total bytes of code: 6

I think it's fine to mark those as AggressiveInlining: Invariant is folded always (inliner has no chance to know it in advance) and IsAsciiCodePoint and ToUpperAsciiInvariant are single instructions basically.

…index # Conflicts: # src/coreclr/inc/jiteeversionguid.h # src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs

EgorBo · 2022-11-20T15:46:19Z

@jkotas does VM side look good to you?

jkotas · 2022-11-20T16:12:36Z

The VM side looks good.

EgorBo · 2022-11-20T16:30:28Z

@SingleAccretion can you take a look again? 🙂

As I said in some of the comments, I decided to avoid assertprop changes in this PR - so it still folds everything, but might leave nullchecks in some cases. I'll work separately (once this lands and collections are updated) to see if it's worth the effort and JIT-TP to clean them up (local runs of jit-diff state that there are not so many hits)

SingleAccretion

Jit side changes LGTM.

EgorBo · 2022-11-20T16:48:24Z

@jakobbotsch Can you please sign off the jit side (if it looks good to you)

src/coreclr/jit/valuenum.cpp

EgorBo · 2022-11-21T00:41:10Z

Merging to have a new collection by Monday.
Two CI failures are unrelated because they're Mono only, e.g. iOS seems to be struggling connecting to a test device in that test (infra issue)

jkotas · 2022-11-21T01:05:22Z

Two CI failures are unrelated because they're Mono only

We still want to keep track of the unrelated intermittent failures to ensure that they are getting fixed: https://github.com/dotnet/runtime/blob/main/docs/pr-builds.md#what-to-do-if-you-determine-the-failure-is-unrelated

Both of the failures are known - I have added comments to the tracking issues for you.

EgorBo · 2022-11-22T14:21:06Z

Improvements on x64:

[Perf] Windows/x64: 2 Improvements on 11/21/2022 12:41:05 AM perf-autofiling-issues#9914
[Perf] Alpine/x64: 2 Improvements on 11/21/2022 12:41:05 AM perf-autofiling-issues#9907
[Perf] Windows/x64: 2 Improvements on 11/21/2022 12:41:05 AM perf-autofiling-issues#10221

Fold "cns"[cns] for span<char>

c0abbd7

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

EgorBo added 2 commits November 19, 2022 16:53

Handle more cases

92a9e38

Fix CI issue

e12b055

EgorBo marked this pull request as ready for review November 19, 2022 19:56

EgorBo requested a review from MichalStrehovsky as a code owner November 19, 2022 19:56

SingleAccretion reviewed Nov 19, 2022

View reviewed changes

EgorBo and others added 2 commits November 19, 2022 23:41

Update src/coreclr/jit/valuenum.cpp

f030653

Co-authored-by: SingleAccretion <[email protected]>

Address feedback

093fd77

EgorBo added 2 commits November 20, 2022 01:13

Clean up

022e744

Mark ToUpperInvariant and ToLowerInvariant with aggressiveinlining

369179d

EgorBo added 5 commits November 20, 2022 02:21

Clean up

548fad8

Don't set NullRef exception set

37dadee

Address SingleAccretion's feedback

fc6f592

Merge branch 'main' of github.com:dotnet/runtime into fold-str-const-…

b406c17

…index # Conflicts: # src/coreclr/inc/jiteeversionguid.h # src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs

Revert post-order const prop for now

8f7f144

TP-change

0c8a8cc

SingleAccretion approved these changes Nov 20, 2022

View reviewed changes

jakobbotsch reviewed Nov 20, 2022

View reviewed changes

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

Update valuenum.cpp

53cbfe8

jakobbotsch approved these changes Nov 20, 2022

View reviewed changes

Add tests

f7d9b29

EgorBo merged commit 0ff0f7d into dotnet:main Nov 21, 2022

EgorBo deleted the fold-str-const-index branch November 21, 2022 00:41

This was referenced Nov 21, 2022

[wasm] S.R.IS.JavaScript.Tests.JSImportExportTest.JsImportTaskTypes #77334

Closed

[iOS] Infrastructure is failing to report successful test run dotnet/arcade#11683

Closed

EgorBo mentioned this pull request Nov 22, 2022

CI test #78700

Closed

lewing mentioned this pull request Nov 22, 2022

[Perf] Linux/x64: 2 Improvements on 11/21/2022 12:41:05 AM dotnet/perf-autofiling-issues#10023

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fold "cns"[cns] for ROS<char> #78593

Fold "cns"[cns] for ROS<char> #78593

EgorBo commented Nov 19, 2022

ghost commented Nov 19, 2022

EgorBo commented Nov 19, 2022 •

edited

Loading

SingleAccretion Nov 19, 2022

EgorBo Nov 20, 2022

SingleAccretion Nov 20, 2022

EgorBo Nov 20, 2022

SingleAccretion Nov 20, 2022

am11 commented Nov 19, 2022

EgorBo commented Nov 20, 2022 •

edited

Loading

EgorBo commented Nov 20, 2022 •

edited

Loading

jkotas commented Nov 20, 2022

EgorBo commented Nov 20, 2022 •

edited

Loading

SingleAccretion left a comment

EgorBo commented Nov 20, 2022

EgorBo commented Nov 21, 2022

jkotas commented Nov 21, 2022

EgorBo commented Nov 22, 2022 •

edited

Loading

	else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic))
	{
	fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]);
	offset = vnStore->ConstantValue<ssize_t>(funcApp.m_args[2]);

	// Note VNF_PtrToStatic statics are currently always "simple".
	fgValueNumberFieldLoad(tree, /* baseAddr */ nullptr, fldSeq, offset);
	}
	else if (tree->OperIs(GT_IND) && fgValueNumberConstLoad(tree->AsIndir()))
	{
	// VN is assigned inside fgValueNumberConstLoad
	}
	else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToArrElem))
	{
	fgValueNumberArrayElemLoad(tree, &funcApp);
	}

Fold "cns"[cns] for ROS<char> #78593

Fold "cns"[cns] for ROS<char> #78593

Conversation

EgorBo commented Nov 19, 2022

ghost commented Nov 19, 2022

EgorBo commented Nov 19, 2022 • edited Loading

SingleAccretion Nov 19, 2022

Choose a reason for hiding this comment

EgorBo Nov 20, 2022

Choose a reason for hiding this comment

SingleAccretion Nov 20, 2022

Choose a reason for hiding this comment

EgorBo Nov 20, 2022

Choose a reason for hiding this comment

SingleAccretion Nov 20, 2022

Choose a reason for hiding this comment

am11 commented Nov 19, 2022

EgorBo commented Nov 20, 2022 • edited Loading

EgorBo commented Nov 20, 2022 • edited Loading

jkotas commented Nov 20, 2022

EgorBo commented Nov 20, 2022 • edited Loading

SingleAccretion left a comment

Choose a reason for hiding this comment

EgorBo commented Nov 20, 2022

EgorBo commented Nov 21, 2022

jkotas commented Nov 21, 2022

EgorBo commented Nov 22, 2022 • edited Loading

EgorBo commented Nov 19, 2022 •

edited

Loading

EgorBo commented Nov 20, 2022 •

edited

Loading

EgorBo commented Nov 20, 2022 •

edited

Loading

EgorBo commented Nov 20, 2022 •

edited

Loading

EgorBo commented Nov 22, 2022 •

edited

Loading