dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.64k forks source link

C# value discard leads to range check for happy path #99943

Open OwnageIsMagic opened 5 months ago

OwnageIsMagic commented 5 months ago

Description

Code:

#nullable disable
using System;
using System.Runtime.CompilerServices;

public class C {
    public static void Main() {
        Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription);
        M(null);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static int M(byte [] a) {
        for (int i = 0; i < 10; ++i) {
            _ = a[i];

            Console.WriteLine(a[i]);
        }

        return 5;
    }
}

running with $env:DOTNET_JitDisasm="M", $env:DOTNET_TieredCompilation=0

Generated code

```asm .NET 9.0.0-preview.2.24128.5 ; Assembly listing for method C:M(ubyte[]):int (FullOpts) ; Emitting BLENDED_CODE for X64 with AVX - Windows ; FullOpts code ; optimized code ; rsp based frame ; partially interruptible ; No PGO data G_M000_IG01: ;; offset=0x0000 push rdi push rsi push rbx sub rsp, 32 mov rbx, rcx G_M000_IG02: ;; offset=0x000A xor esi, esi test rbx, rbx ;; I don't quite understand for what this null check is for, since it jumps to the same loop, but it's another issue je SHORT G_M000_IG06 G_M000_IG03: ;; offset=0x0011 mov edi, dword ptr [rbx+0x08] cmp edi, 10 jl SHORT G_M000_IG06 G_M000_IG04: ;; offset=0x0019 cmp esi, edi ;; range check on happy path :( jae SHORT G_M000_IG09 mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] inc esi cmp esi, 10 jl SHORT G_M000_IG04 G_M000_IG05: ;; offset=0x0031 jmp SHORT G_M000_IG07 G_M000_IG06: ;; offset=0x0033 mov edi, dword ptr [rbx+0x08] cmp esi, edi jae SHORT G_M000_IG09 mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] inc esi cmp esi, 10 jl SHORT G_M000_IG06 G_M000_IG07: ;; offset=0x004E mov eax, 5 G_M000_IG08: ;; offset=0x0053 add rsp, 32 pop rbx pop rsi pop rdi ret G_M000_IG09: ;; offset=0x005B call CORINFO_HELP_RNGCHKFAIL int3 ; Total bytes of code 97 ```

There is G_M000_IG04 section is identical to G_M000_IG06, both contain range check despite the fact that G_M000_IG03 should guard against it.

Using a[i] normally (i.e. second System.Console.WriteLine(a[i]); correctly omits range check.

Correct example

For ```cs for (int i = 0; i < 10; ++i) { Console.WriteLine(a[i]); Console.WriteLine(a[i]); } ``` Generated code (excerpt) ```asm G_M000_IG03: ;; offset=0x0010 cmp dword ptr [rbx+0x08], 10 jl SHORT G_M000_IG06 G_M000_IG04: ;; offset=0x0016 mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] inc esi cmp esi, 10 jl SHORT G_M000_IG04 G_M000_IG05: ;; offset=0x0037 jmp SHORT G_M000_IG07 G_M000_IG06: ;; offset=0x0039 cmp esi, dword ptr [rbx+0x08] jae SHORT G_M000_IG09 mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] mov ecx, esi movzx rcx, byte ptr [rbx+rcx+0x10] call [System.Console:WriteLine(int)] inc esi cmp esi, 10 jl SHORT G_M000_IG06 ```

dotnet-policy-service[bot] commented 5 months ago

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

jakobbotsch commented 5 months ago

Looks like various optimization phases do not recognize top-most BOUNDS_CHECK nodes.

OwnageIsMagic commented 5 months ago

I know, JIT is bad at optimizing stupid code due to limited time budget and worse debugging experience, but what is team stand on code quality for corner cases now, with tiered jit on by default?