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

Skip covariant checks for ldelema T[] with T exact #85256

Merged
merged 1 commit into from Apr 24, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 24, 2023

Closes #65789

public sealed class C
{
    C[] _array;

    C Test() => Volatile.Read(ref _array[0]);
}

Was:

; Assembly listing for method C:Test():C:this
       sub      rsp, 40
       mov      rcx, gword ptr [rcx+08H]
       xor      edx, edx
       mov      r8, 0x7FF85CB4CE28      ; C
       call     CORINFO_HELP_LDELEMA_REF
       mov      rax, gword ptr [rax]
       add      rsp, 40
       ret      
; Total bytes of code 33

Now:

; Assembly listing for method C:Test():C:this
       sub      rsp, 40
       mov      rax, gword ptr [rcx+08H]
       mov      edx, dword ptr [rax+08H]
       test     rdx, rdx
       je       SHORT G_M7456_IG04
       mov      rax, gword ptr [rax+10H]
       add      rsp, 40
       ret      
G_M7456_IG04:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code 31

@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 Apr 24, 2023
@ghost ghost assigned EgorBo Apr 24, 2023
@ghost
Copy link

ghost commented Apr 24, 2023

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

Issue Details

Closes #65789

public sealed class C
{
    C[] _array;

    C Test() => Volatile.Read(ref _array[0]);
}

Was:

; Assembly listing for method C:Get1():C:this
       sub      rsp, 40
       mov      rcx, gword ptr [rcx+08H]
       xor      edx, edx
       mov      r8, 0x7FF85CB4CE28      ; C
       call     CORINFO_HELP_LDELEMA_REF
       mov      rax, gword ptr [rax]
       add      rsp, 40
       ret      
; Total bytes of code 33

Now:

; Assembly listing for method C:Get1():C:this
       sub      rsp, 40
       mov      rax, gword ptr [rcx+08H]
       mov      edx, dword ptr [rax+08H]
       test     rdx, rdx
       je       SHORT G_M7456_IG04
       mov      rax, gword ptr [rax+10H]
       add      rsp, 40
       ret      
G_M7456_IG04:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code 31
Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review April 24, 2023 16:29
@EgorBo
Copy link
Member Author

EgorBo commented Apr 24, 2023

A few diffs. Size regressions on arm64 because of "heavier" bound checks, but should be clean perf improvement/

@dotnet/jit-contrib @AndyAyersMS PTAL

@EgorBo EgorBo requested a review from AndyAyersMS April 24, 2023 17:15
@@ -7210,7 +7208,19 @@ void Compiler::impImportBlockCode(BasicBlock* block)
// The array helper takes a native int for array length.
// So if we have an int, explicitly extend it to be a native int.
index = impImplicitIorI4Cast(index, TYP_I_IMPL);
op1 = gtNewHelperCallNode(CORINFO_HELP_LDELEMA_REF, TYP_BYREF, arr, index, type);

if ((gtGetArrayElementClassHandle(arr) == ldelemClsHnd) && impIsClassExact(ldelemClsHnd))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that unverifiable/unsafe code, or specifically code with runtime behavior that actually deviates from the static types, has undefined behavior. In other words, we don't need to throw the exception on an object[] that has been misrepresented as a string[] and then a ldelem.a string performed on it. (This makes sense since such optimizations would then be impossible; it's just against a strict reading of the specification.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be conditional on opts.OptimizationEnabled() ? I see you had a minopts diff...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be conditional on opts.OptimizationEnabled() ? I see you had a minopts diff...

Addressed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, we don't need to throw the exception on an object[] that has been misrepresented as a string[] and then a ldelem.a string performed on it.

I'm not sure what is going to happen (and what is expected) in this case so I'm only optimizing a valid case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would optimize that case. Something like

.locals string[] myvar

newarr obj
stloc myvar // type inconsistency, assigning object[] to string[]
// maybe more code to block any kind of optimization between the stloc and ldloc
ldloc myvar
ldc.whatever
ldelema string

or even worse if unsafe part is in a caller... but, as I said, I assume that this isn't a concern because otherwise all type information is useless but just wanted to bring it up in case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this case should fail on the stloc step

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, do we reject (at least some kinds of) unverifiable code now? This is just moving an object reference around; it's only if one looks at the so-called verification types does it break down. (I guess I should just try it.. I'd be happy to learn this.)

This definitely could be done with Unsafe.As, but the docs there are clear about improper use being undefined.

assert(lclTyp == TYP_REF);
block->bbFlags |= BBF_HAS_IDX_LEN;
optMethodFlags |= OMF_HAS_ARRAYREF;
op1 = gtNewArrayIndexAddr(arr, index, lclTyp, ldelemClsHnd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the standard pattern here is to goto ARR_LD. It would require the pop to be split to impStackTop/impPopStack(2) but pick up the various cases there (unless you're intentionally skipping those here?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Addressed

@EgorBo
Copy link
Member Author

EgorBo commented Apr 24, 2023

SPMI failures are due to recent JIT/EE guid change

@EgorBo EgorBo merged commit f62bb9f into dotnet:main Apr 24, 2023
116 of 125 checks passed
@EgorBo EgorBo deleted the ldelema-covariant branch April 24, 2023 21:27
@dotnet dotnet locked as resolved and limited conversation to collaborators May 25, 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 this pull request may close these issues.

Unnecessary LdelemaRef call not elided with Volatile.Read
3 participants