Skip to content

Conversation

@sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Apr 5, 2021

Fixes #44730

In the past, we did not see REF types from ldobj handle, we had:

    [ 0]   4 (0x004) ldloca.s 0
    [ 1]   6 (0x006) ldobj pointer 
    [ 1]  11 (0x00b) ret

and jit treated it like a value type with POINTER_SIZE(in this case 8-byte, so used long):

               [000010] *--XG-------              *  IND       long  
               [000009] ------------              \--*  ADDR      byref 
               [000008] -------N----                 \--*  LCL_VAR   long   V01 tmp1 

after the VM change we see the real REF type (a pointer to an object on a heap):

               [000010] *--XG-------              *  IND       ref   
               [000009] ------------              \--*  ADDR      long  
               [000008] -------N----                 \--*  LCL_VAR   long   V01 tmp1 

Note that we allow REF->IMPL conversion, but not IMPL->REF, cc @AndyAyersMS .

I guess I need to revert part of #44465 but not sure what exactly.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 5, 2021
@sandreenko
Copy link
Contributor Author

PTAL @jkotas, @AndyAyersMS , cc @MichalStrehovsky @dotnet/jit-contrib

@sandreenko sandreenko marked this pull request as ready for review April 6, 2021 02:09
@sandreenko
Copy link
Contributor Author

There are no diffs crossgen/pmi on --f. I have manually collected dasm for base and diff using different VM versions and checked with jit-analyze.

@jkotas
Copy link
Member

jkotas commented Apr 11, 2021

Could you please also make a matching change in src\coreclr\tools\Common\JitInterface\CorInfoImpl.cs:

Change isValueClass to just:

        private bool isValueClass(CORINFO_CLASS_STRUCT_* cls)
        {
            return HandleToObject(cls).IsValueType;
        }

And delete this from getClassAttribsInternal:

            // CoreCLR uses UIntPtr in place of pointers here
            if (type.IsPointer || type.IsFunctionPointer)
                type = _compilation.TypeSystemContext.GetWellKnownType(WellKnownType.UIntPtr);

The VM side of the change looks good to me otherwise.

@sandreenko sandreenko changed the title Test a fix for "IsValueClass". Fix JITEEInterface::IsValueClass to return false for pointers. Apr 12, 2021
@sandreenko
Copy link
Contributor Author

ping @dotnet/jit-contrib

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

This will need a new jit GUID.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM.

@sandreenko sandreenko merged commit 5a2df34 into dotnet:main Apr 13, 2021
@sandreenko sandreenko deleted the FixisValueClass branch April 13, 2021 07:42
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
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.

Consider fixing semantics of JIT/EE interface isValueClass method

4 participants