Difference in codegen of string.IsNullOrEmpty()

[ShreyasJejurkar]

I was playing with JIT a little bit recently, and found the below scenario where I am checking does give string is null or empty with two different styles,

1. With raw null and length check.
2. and with string.IsNullOrEmpty() built-in method, which does the same raw null and length check behind the scenes.

But the collagen is a little different for both cases!
For given a program below (corresponding sharplab link)

using System;
public class C {
    public void M() {
        var someString = "";
        
        if(someString == null || someString.Length == 0) {
            Console.Write("hello");
        }
    }
    
    public void T() {
        string someString = "";
        
        if(string.IsNullOrEmpty(someString)) {
            Console.Write("hello");
        }
    }
}

Following is the resulting codegen

C.M()
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov ecx, [0x8d72018]
    L0009: cmp dword ptr [ecx+4], 0
    L000d: jne short L001a
    L000f: mov ecx, [0x8dce1d8]
    L0015: call System.Console.Write(System.String)
    L001a: pop ebp
    L001b: ret

C.T()
    L0000: mov ecx, [0x8dce1d8]
    L0006: call System.Console.Write(System.String)
    L000b: ret

As we can see above, codegen for string.IsNullOrEmpty(string) is quite optimized, so my question is can we do the same optimization for the raw null check call as well (OR maybe fold that raw call with the built-in method by the compiler), given the fact that the built-in also does the same behind the scenes, but still codegen is quite different!

[ghost]

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

Issue Details

---

I was playing with JIT a little bit recently, and found the below scenario where I am checking does give string is null or empty with two different styles,

1. With raw null and length check.
2. and with string.IsNullOrEmpty() built-in method, which does the same raw null and length check behind the scenes.

But the collagen is a little different for both cases!
For given a program below (corresponding sharplab link)

using System;
public class C {
    public void M() {
        var someString = "";
        
        if(someString == null || someString.Length == 0) {
            Console.Write("hello");
        }
    }
    
    public void T() {
        string someString = "";
        
        if(string.IsNullOrEmpty(someString)) {
            Console.Write("hello");
        }
    }
}

Following is the resulting codegen

C.M()
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov ecx, [0x8d72018]
    L0009: cmp dword ptr [ecx+4], 0
    L000d: jne short L001a
    L000f: mov ecx, [0x8dce1d8]
    L0015: call System.Console.Write(System.String)
    L001a: pop ebp
    L001b: ret

C.T()
    L0000: mov ecx, [0x8dce1d8]
    L0006: call System.Console.Write(System.String)
    L000b: ret

As we can see above, codegen for string.IsNullOrEmpty(string) is quite optimized, so my question is can we do the same optimization for the raw null check call as well (OR maybe fold that raw call with the built-in method by the compiler), given the fact that the built-in also does the same behind the scenes, but still codegen is quite different!

Author:	ShreyasJejurkar
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[huoyaoyuan]

See #63095 and #4207

[ShreyasJejurkar]

I can see your PR linked to the issue, will that PR will fix this issue!?
I saw some regression in diff!

[EgorBo]

@ShreyasJejurkar in your T method jit sees that someString is a string literal which is never null so it folded everything

[ShreyasJejurkar]

Thanks @EgorBo for the reply! Won't JIT see same thing for method M because it is the same thing there as well and can fold?

[EgorBo]

    public void M() {
        var someString = "";
        
        if(someString == null || someString.Length == 0) {
            Console.Write("hello");
        }
    }

Ah, I missed that part.
Yes, it's a known problem in jit - it needs "Forward Substitution" feature to handle cases like this one. Honestly, I thought Roslyn always propagated ldstr by value 🤔

[ShreyasJejurkar]

Even more what got me into thinking that, the string.IsNullOrEmpty() under the hood implementation is the same as method M, but still it did not fold! https://source.dot.net/#System.Private.CoreLib/String.cs,486

[EgorBo]

Even more what got me into thinking that, the string.IsNullOrEmpty() under the hood implementation is the same as method M, but still it did not fold! https://source.dot.net/#System.Private.CoreLib/String.cs,486

It's because JIT actually supports "Forward substitution" but only when it inlines functions.

[ShreyasJejurkar]

I created a new version of the same IsNullOrEmpty() with the same implementation and with aggressive inline, but still codegen is not optimized, not sure why JIT cannot inline that method as well!

using System;
using System.Runtime.CompilerServices;
public class C {
    
    public void M() {
        var someString = "";
        
        if(IsNullOrEmptyVer(someString)) {
            Console.Write("hello");
        }
    }
    
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public bool IsNullOrEmptyVer(string s) {
        return s == null || s.Length == 0;
    }
    
    
    public void T() {
        string someString = "";
        
        if(string.IsNullOrEmpty(someString)) {
            Console.Write("hello");
        }
    }
}

SharpLab link -> https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgAwAEmuAdAEoCuMCUYAplQMID2YADlADYsATgGVhANygBjFigDcRTAGYK2MuzIBvImV1kde5RQAsZALIAKAJRaDeveIhCyKbixEIhsAOZkAvGQARIEKhPb2duFQAGYWAJIoAHJ0/PwA8kIAorwIAJ4AasIWrqweXjDeVjbaYeF1XDCuglQA6l4ILBaBABYsqZyBVqF1ugC+keO1upEA2mYsCN2cACZxvPwW84srazzpPEycjVQAgt7eQrIoUOIscTD8sD5WALqRRsCcnPxkCcmpGWyBwKRUo5BQ1Ui9kwAHYXP4AjAUj90OgXFQADIsCqLBFkEjDPSTCJTfSkoyYUwAFWstlJ0Nw4LcZR8/iCIShek5uhiFjBVD+yMBOVyxWZnmekPpIwaTTYbSgHS6vX6g0JdWJRKIoyAA===

[ShreyasJejurkar]

I saw this issue about "Forward Substitution", but it's dead I guess! #6973

[EgorBo]

@ShreyasJejurkar it's Roslyn - it decided to save your "" into a temp. Make your IsNullOrEmptyVer static.

        IL_0000: ldstr ""
        IL_0005: stloc.0
        IL_0006: ldarg.0
        IL_0007: ldloc.0
        IL_0008: call instance bool C::IsNullOrEmptyVer(string)

but when you make it static:

        IL_0000: ldstr ""
        IL_0005: call bool C::IsNullOrEmptyVer(string)

[ShreyasJejurkar]

Thanks @EgorBo , it did. SharpLab link

[EgorBo]

NOTE: I am not blaming Roslyn here 🙂 - it's a correct IL and it's jit's fault that it doesn't do forward sub.

[ShreyasJejurkar]

@EgorBo ahh, it's ok. No one to blame here, just an opportunity.
Are there any plans to address this kind of issue!?

[EgorBo]

Are there any plans to address this kind of issue!?

Soon or later 🙂

[EgorBo]

Closing since it has expected codegen now (apparently, via Fsub)