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

CodeGen difference when manually extracting element to a local #102518

Closed
MihaZupan opened this issue May 21, 2024 · 2 comments · Fixed by #102808
Closed

CodeGen difference when manually extracting element to a local #102518

MihaZupan opened this issue May 21, 2024 · 2 comments · Fixed by #102808
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented May 21, 2024

SharpLab

public static void Test1(ReadOnlySpan<char> s) => s.StartsWith1('a');

public static void Test2(ReadOnlySpan<char> s) => s.StartsWith2('a');

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool StartsWith1<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?
{
    if (span.Length > 0)
    {
        return span[0]?.Equals(value) ?? (object?)value is null;
    }
    return false;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool StartsWith2<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T>?
{
    if (span.Length > 0)
    {
        T temp = span[0];
        return temp?.Equals(value) ?? (object?)value is null;
    }
    return false;
}
TestClass.Test1(System.ReadOnlySpan`1<Char>)
    L0000: push rax
    L0001: mov rax, [rcx]
    L0004: mov ecx, [rcx+8]
    L0007: xor edx, edx
    L0009: mov [rsp+4], edx
    L000d: test ecx, ecx
    L000f: jle short L0019
    L0011: movzx eax, word ptr [rax]
    L0014: mov [rsp+4], ax
    L0019: add rsp, 8
    L001d: ret

TestClass.Test2(System.ReadOnlySpan`1<Char>)
    L0000: mov rax, [rcx]
    L0003: mov ecx, [rcx+8]
    L0006: test ecx, ecx
    L0008: jle short L000d
    L000a: cmp [rax], ax
    L000d: ret

Wasn't sure what to search for to look if there's an existing issue for this.

@MihaZupan MihaZupan added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 21, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 21, 2024
Copy link
Contributor

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

@jakobbotsch
Copy link
Member

This is the same underlying problem as #87072 and #102273, where the IL takes the address of a local and leaves it live on the IL stack across control flow.
It's on my todo list to fix this for .NET 9.

@jakobbotsch jakobbotsch self-assigned this May 21, 2024
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone May 21, 2024
@jakobbotsch jakobbotsch added Priority:2 Work that is important, but not critical for the release and removed untriaged New issue has not been triaged by the area owner labels May 21, 2024
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue May 29, 2024
This changes local morph to run in RPO when optimizations are enabled.
It adds infrastructure to track and propagate LCL_ADDR values assigned
to locals (or struct fields) during local morph. This allows us to avoid
address exposure in cases where the destination local does not actually
end up escaping in any way.

Example:
```csharp
public struct Awaitable
{
    public int Opts;

    public Awaitable(bool value)
    {
        Opts = value ? 1 : 2;
    }
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static int Test() => new Awaitable(false).Opts;
```

Before:
```asm
G_M59043_IG01:  ;; offset=0x0000
       push     rax
						;; size=1 bbWeight=1 PerfScore 1.00

G_M59043_IG02:  ;; offset=0x0001
       xor      eax, eax
       mov      dword ptr [rsp], eax
       mov      dword ptr [rsp], 2
       mov      eax, dword ptr [rsp]
						;; size=15 bbWeight=1 PerfScore 3.25

G_M59043_IG03:  ;; offset=0x0010
       add      rsp, 8
       ret
						;; size=5 bbWeight=1 PerfScore 1.25
; Total bytes of code: 21

```

After:
```asm
G_M59043_IG02:  ;; offset=0x0000
       mov      eax, 2
						;; size=5 bbWeight=1 PerfScore 0.25

G_M59043_IG03:  ;; offset=0x0005
       ret
```

Propagating the addresses works much like local assertion prop in morph
does. Proving that the locations that were stored to do not escape
afterwards is done with a simplistic approach: we check globally that no
reads of the location exists, and if so, we replace the `LCL_ADDR`
stored to them by a constant 0. We leave it up to liveness to clean up
the stores themselves.

This could be more sophisticated, but in practice this handles the
reported cases just fine.

If we were able to remove any `LCL_ADDR` in this way then we run an
additional pass over the locals of the IR to compute the final set of
exposed locals.

Fix dotnet#87072
Fix dotnet#102273
Fix dotnet#102518

This is still not sufficient to handle dotnet#69254. To handle that case we
need to handle transferring assertions for struct copies, and also
handle proving that specific struct fields containing local addresses do
not escape. It is probably doable, but for now I will leave it as future
work.
@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2024
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 Priority:2 Work that is important, but not critical for the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants