Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe.As breaks callvirt after inlining in .NET 8 RC2 #93650

Closed
pentp opened this issue Oct 18, 2023 · 2 comments · Fixed by #93694
Closed

Unsafe.As breaks callvirt after inlining in .NET 8 RC2 #93650

pentp opened this issue Oct 18, 2023 · 2 comments · Fixed by #93694
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Comments

@pentp
Copy link
Contributor

pentp commented Oct 18, 2023

Description

If a call to Unsafe.As<A, B>() and another call that uses the result (e.g., ToString()) are inlined by JIT, it results in bad codegen. If inlined manually, codegen is correct.

Reproduction Steps

Bad codegen sample: https://godbolt.org/z/YEoYM7xY5

Minimal repro:

using System.Runtime.CompilerServices;
using System.Text;

internal static class Program
{
    static void Main()
    {
        var sb = new Holder();
        while (true)
        {
            var s = Bind(ref sb);
            if (s.Length != 0)
            {
                Console.WriteLine("StringBuilder.ToString() returned: " + s);
                return;
            }
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static string Bind(ref Holder parameters) => GetString(parameters.GetBuilder());

    public static string GetString(StringBuilder sb) => sb.ToString();
}

internal struct Holder
{
    internal StringBuilder.AppendInterpolatedStringHandler _h;
    public Holder() => _h = new(0, 0, new());

    internal StringBuilder GetBuilder() => Unsafe.As<StringBuilder.AppendInterpolatedStringHandler, StringBuilder>(ref _h);
}

Expected behavior

StringBuilder:ToString() is called (asm listing after manually inlining GetString):

; Assembly listing for method Program:Bind(byref):System.String (Tier1)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; Tier1 code
; optimized code
; rsp based frame
; partially interruptible
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 40

G_M000_IG02:                ;; offset=0x0004
       mov      rcx, gword ptr [rcx]
       cmp      dword ptr [rcx], ecx
       call     [System.Text.StringBuilder:ToString():System.String:this]
       nop

G_M000_IG03:                ;; offset=0x0010
       add      rsp, 40
       ret

Actual behavior

Object:ToString() is inlined instead:

; Assembly listing for method Program:Bind(byref):System.String (Tier1)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; Tier1 code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 3 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 40

G_M000_IG02:                ;; offset=0x0004
       mov      rcx, gword ptr [rcx]
       cmp      byte  ptr [rcx], cl
       call     System.Object:GetType():System.Type:this
       mov      rcx, rax
       mov      edx, 1
       call     [System.RuntimeType:GetCachedName(int):System.String:this]
       nop

G_M000_IG03:                ;; offset=0x001D
       add      rsp, 40
       ret

Regression?

This worked correctly in .NET 7

Known Workarounds

No response

Configuration

.NET 8.0.100-rc.2.23502.2, Windows x64

Other information

No response

@dotnet-issue-labeler 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 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

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

Issue Details

Description

If a call to Unsafe.As<A, B>() and another call that uses the result (e.g., ToString()) are inlined by JIT, it results in bad codegen. If inlined manually, codegen is correct.

Reproduction Steps

Bad codegen sample: https://godbolt.org/z/YEoYM7xY5

Minimal repro:

using System.Runtime.CompilerServices;
using System.Text;

internal static class Program
{
    static void Main()
    {
        var sb = new Holder();
        while (true)
        {
            var s = Bind(ref sb);
            if (s.Length != 0)
            {
                Console.WriteLine("StringBuilder.ToString() returned: " + s);
                return;
            }
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static string Bind(ref Holder parameters) => GetString(parameters.GetBuilder());

    public static string GetString(StringBuilder sb) => sb.ToString();
}

internal struct Holder
{
    internal StringBuilder.AppendInterpolatedStringHandler _h;
    public Holder() => _h = new(0, 0, new());

    internal StringBuilder GetBuilder() => Unsafe.As<StringBuilder.AppendInterpolatedStringHandler, StringBuilder>(ref _h);
}

Expected behavior

StringBuilder:ToString() is called (asm listing after manually inlining GetString):

; Assembly listing for method Program:Bind(byref):System.String (Tier1)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; Tier1 code
; optimized code
; rsp based frame
; partially interruptible
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 40

G_M000_IG02:                ;; offset=0x0004
       mov      rcx, gword ptr [rcx]
       cmp      dword ptr [rcx], ecx
       call     [System.Text.StringBuilder:ToString():System.String:this]
       nop

G_M000_IG03:                ;; offset=0x0010
       add      rsp, 40
       ret

Actual behavior

Object:ToString() is inlined instead:

; Assembly listing for method Program:Bind(byref):System.String (Tier1)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; Tier1 code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 3 single block inlinees; 0 inlinees without PGO data

G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 40

G_M000_IG02:                ;; offset=0x0004
       mov      rcx, gword ptr [rcx]
       cmp      byte  ptr [rcx], cl
       call     System.Object:GetType():System.Type:this
       mov      rcx, rax
       mov      edx, 1
       call     [System.RuntimeType:GetCachedName(int):System.String:this]
       nop

G_M000_IG03:                ;; offset=0x001D
       add      rsp, 40
       ret

Regression?

This worked correctly in .NET 7

Known Workarounds

No response

Configuration

.NET 8.0.100-rc.2.23502.2, Windows x64

Other information

No response

Author: pentp
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion
Copy link
Contributor

The bug is that gtGetClassHandle gets confused by the indirect access induced with Unsafe.As.

There is an ambiguity when querying field classes for IND(FIELD_ADDR): do you have a wrapper struct, or are you looking at the genuine TYP_REF field.

The proposed fix is to not return anything for not-TYP_REF fields from gtGetFieldClassHandle:

diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp
index 81b6bbdc85c..0009c9045aa 100644
--- a/src/coreclr/jit/gentree.cpp
+++ b/src/coreclr/jit/gentree.cpp
@@ -18716,9 +18716,11 @@ CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldH
                 JITDUMP("Field's current class not available\n");
             }
         }
+
+        return fieldClass;
     }

-    return fieldClass;
+    return NO_CLASS_HANDLE;
 }

@AndyAyersMS AndyAyersMS self-assigned this Oct 18, 2023
@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Oct 18, 2023
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Oct 18, 2023
Thanks to @SingleAccretion for the fix.

The JIT was assuming that an indirectly accessed value type field had
the type of the field, but that might not be the case if the field
was accessed via `Unsafe.As`.

Fix this by limiting type deduction from these indirectly accessed fields
to only ref type fields.

Closes dotnet#93650.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 18, 2023
AndyAyersMS added a commit that referenced this issue Oct 18, 2023
Thanks to @SingleAccretion for the fix.

The JIT was assuming that an indirectly accessed value type field had
the type of the field, but that might not be the case if the field
was accessed via `Unsafe.As`.

Fix this by limiting type deduction from these indirectly accessed fields
to only ref type fields.

Closes #93650.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 18, 2023
github-actions bot pushed a commit that referenced this issue Oct 18, 2023
Thanks to @SingleAccretion for the fix.

The JIT was assuming that an indirectly accessed value type field had
the type of the field, but that might not be the case if the field
was accessed via `Unsafe.As`.

Fix this by limiting type deduction from these indirectly accessed fields
to only ref type fields.

Closes #93650.
carlossanlop pushed a commit that referenced this issue Oct 20, 2023
Thanks to @SingleAccretion for the fix.

The JIT was assuming that an indirectly accessed value type field had
the type of the field, but that might not be the case if the field
was accessed via `Unsafe.As`.

Fix this by limiting type deduction from these indirectly accessed fields
to only ref type fields.

Closes #93650.

Co-authored-by: Andy Ayers <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants