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.

Sign up for GitHub

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

Jump to bottom

JIT: Fold typeof(T).TypeHandle.Value #85804

Merged
merged 18 commits into from May 11, 2023
Merged

JIT: Fold typeof(T).TypeHandle.Value #85804

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
52065e1
JIT: Fold typeof(T).TypeHandle.Value
EgorBo May 5, 2023
04210ec
Handle NativeAOT
EgorBo May 5, 2023
3c81b79
Clean up
EgorBo May 5, 2023
e4d391b
fix test
EgorBo May 5, 2023
375df02
check callvirt explicitly
EgorBo May 5, 2023
9cd3880
Merge branch 'main' of github.com:dotnet/runtime into fold-typehandle…
EgorBo May 6, 2023
7716934
Address feedback
EgorBo May 7, 2023
726a42f
formatting
EgorBo May 7, 2023
5b313af
formatting
EgorBo May 7, 2023
5bdfc2c
Address feedback
EgorBo May 8, 2023
7821c85
address feedback
EgorBo May 8, 2023
7ab11e5
oops, reverted too much
EgorBo May 8, 2023
ec522f2
Addressed feedback
EgorBo May 8, 2023
83a6fae
Merge branch 'main' into fold-typehandle-value
EgorBo May 9, 2023
473a361
Update src/coreclr/jit/importercalls.cpp
EgorBo May 9, 2023
eaeaef5
Merge branch 'main' of github.com:dotnet/runtime into fold-typehandle…
EgorBo May 10, 2023
0442988
formatting
EgorBo May 10, 2023
0e4b625
address feedback
EgorBo May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 4 additions & 11 deletions src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs
View file
Open in desktop
Expand Up @@ -671,18 +671,11 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification)
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
}

// kept outside of the small arrays hot path to have inlining without big size growth
return AllocateNewUninitializedArray(length, pinned);

// remove the local function when https://github.com/dotnet/runtime/issues/5973 is implemented
static T[] AllocateNewUninitializedArray(int length, bool pinned)
{
GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL;
if (pinned)
flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;
GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
if (pinned)
flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;

return Unsafe.As<T[]>(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags));
}
return Unsafe.As<T[]>(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags));
}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Expand Up @@ -3948,6 +3948,7 @@ class Compiler
int memberRef,
bool readonlyCall,
bool tailCall,
bool callvirt,
CORINFO_RESOLVED_TOKEN* pContstrainedResolvedToken,
CORINFO_THIS_TRANSFORM constraintCallThisTransform,
NamedIntrinsic* pIntrinsicName,
Expand Down
67 changes: 60 additions & 7 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Expand Up @@ -233,9 +233,9 @@ var_types Compiler::impImportCall(OPCODE opcode,

const bool isTailCall = canTailCall && (tailCallFlags != 0);

call =
impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token, isReadonlyCall,
isTailCall, pConstrainedResolvedToken, callInfo->thisTransform, &ni, &isSpecialIntrinsic);
call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token, isReadonlyCall,
isTailCall, opcode == CEE_CALLVIRT, pConstrainedResolvedToken, callInfo->thisTransform,
&ni, &isSpecialIntrinsic);

if (compDonotInline())
{
Expand Down Expand Up @@ -2304,6 +2304,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
int memberRef,
bool readonlyCall,
bool tailCall,
bool callvirt,
CORINFO_RESOLVED_TOKEN* pConstrainedResolvedToken,
CORINFO_THIS_TRANSFORM constraintCallThisTransform,
NamedIntrinsic* pIntrinsicName,
Expand Down Expand Up @@ -2589,7 +2590,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant:

// We need these to be able to fold "typeof(...) == typeof(...)"
case NI_System_RuntimeTypeHandle_ToIntPtr:
case NI_System_Type_GetTypeFromHandle:
case NI_System_Type_op_Equality:
case NI_System_Type_op_Inequality:
Expand All @@ -2614,6 +2614,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
case NI_System_BitConverter_SingleToInt32Bits:
case NI_System_Buffers_Binary_BinaryPrimitives_ReverseEndianness:
case NI_System_Type_GetEnumUnderlyingType:
case NI_System_Type_get_TypeHandle:
case NI_System_RuntimeType_get_TypeHandle:
case NI_System_RuntimeTypeHandle_ToIntPtr:

// Most atomics are compiled to single instructions
case NI_System_Threading_Interlocked_And:
Expand Down Expand Up @@ -2970,8 +2973,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
case NI_System_RuntimeTypeHandle_ToIntPtr:
{
GenTree* op1 = impStackTop(0).val;
if (op1->gtOper == GT_CALL && (op1->AsCall()->gtCallType == CT_HELPER) &&
gtIsTypeHandleToRuntimeTypeHandleHelper(op1->AsCall()))

if (op1->IsHelperCall() && gtIsTypeHandleToRuntimeTypeHandleHelper(op1->AsCall()))
{
// Old tree
// Helper-RuntimeTypeHandle -> TreeToGetNativeTypeHandle
Expand All @@ -2989,7 +2992,25 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
op1 = op1->AsCall()->gtArgs.GetArgByIndex(0)->GetNode();
retNode = op1;
}
// Call the regular function.
else if (op1->OperIs(GT_RET_EXPR))
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
// Skip roundtrip "handle -> RuntimeType -> handle" for
// RuntimeTypeHandle.ToIntPtr(typeof(T).TypeHandle)
GenTreeCall* call = op1->AsRetExpr()->gtInlineCandidate;
if (lookupNamedIntrinsic(call->gtCallMethHnd) == NI_System_RuntimeType_get_TypeHandle)
{
// Check that the arg is CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE helper call
GenTree* arg = call->gtArgs.GetArgByIndex(0)->GetNode();
if (arg->IsHelperCall() && gtIsTypeHandleToRuntimeTypeHelper(arg->AsCall()))
{
impPopStack();
// Bash the RET_EXPR to no-op since it's unused now
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
op1->AsRetExpr()->gtInlineCandidate->gtBashToNOP();
// Skip roundtrip and return the type handle directly
retNode = arg->AsCall()->gtArgs.GetArgByIndex(0)->GetNode();
}
}
}
break;
}

Expand Down Expand Up @@ -3089,6 +3110,30 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
break;
}

case NI_System_Type_get_TypeHandle:
{
// We can only expand this on NativeAOT where RuntimeTypeHandle looks like this:
//
// struct RuntimeTypeHandle { IntPtr _value; }
//
GenTree* op1 = impStackTop(0).val;
if (IsTargetAbi(CORINFO_NATIVEAOT_ABI) && op1->IsHelperCall() &&
gtIsTypeHandleToRuntimeTypeHelper(op1->AsCall()) && callvirt)
{
assert(info.compCompHnd->getClassNumInstanceFields(sig->retTypeClass) == 1);

unsigned structLcl = lvaGrabTemp(true DEBUGARG("RuntimeTypeHandle"));
lvaSetStruct(structLcl, sig->retTypeClass, false);
GenTree* realHandle = op1->AsCall()->gtArgs.GetUserArgByIndex(0)->GetNode();
GenTreeLclFld* handleFld = gtNewLclFldNode(structLcl, realHandle->TypeGet(), 0);
GenTree* asgHandleFld = gtNewAssignNode(handleFld, realHandle);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to create this as CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE helper call that takes the first arg of op1 as an input?

We have other places in the JIT that look for CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE. I do not think they will be able to recognize this.

Copy link
Member Author

@EgorBo EgorBo May 5, 2023 •
edited

Choose a reason for hiding this comment

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

It probably makes sense but we'll need to fold it back to a constant somewhere later, I tried to do this and hit no diffs (I'm running ILC for a complex app) so maybe we better teach jit to recognize raw handles if we ever find a use case?

Presumably, there is nothing JIT can do additionally for typeof(T).TypeHandle.Value; if it's compared against a different type handle - it will be folded (if possible) anyway

Copy link
Member

Choose a reason for hiding this comment

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

The canonical shape for creating RuntimeTypeHandle from raw handle used in other places is CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE helper. I think we should stick to canonical shapes where possible, to allow pattern matching optimizations to compose.

Cleaning up CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE in later stages applies to all places that use canonical shape for creating RuntimeTypeHandle from raw handle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the change you wanted to see EgorBo@55279bc

It seems to be not needed for CoreCLR since we have to wrap RuntimeType object and I've ran SPMI searching for CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEHANDLE(class handle) and found 0 cases.

I spent some time trying to find a case where this helps and wasn't able to do so, I only see a worse codegen (a stack spill I can't explain) - my understanding that we promote structs right after importer so it's better to expand them there.

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 that the right way to do this optimization is without any AOT special-casing and without RuntimeType special-casing in getObjectContent. I think you are saying that it is complicated and not worth it, and that you would prefer to either do nothing or do some of the special casing.

@dotnet/jit-contrib Any opinions about this?

Copy link
Member Author

@EgorBo EgorBo May 7, 2023 •
edited

Choose a reason for hiding this comment

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

Right, I just feel that the energy is better to spent elsewhere since:

  1. Does NativeAOT needs this optimization? It doesn't look so since GC.NativeAOT.cs doesn't use it (although. it's fully implemented in this PR including shared generics)
  2. Is this pattern popular? Only one hit in GC.CoreCLR.cs in our repo + a few hits in OSS judging by grep.app

So we only miss the shared generic case on CoreCLR and unloadable ALC (on CoreCLR). To support both of them we need to mark two methods with [Intrinsic] (Type.get_TypeHandle and RuntimeTypeHandle.get_Value (before it is inlined) - in this case we also will need some hacky way to work with a spilled struct RuntimeTypeHandle before we can detect the shape from two intrinsics.

STMT00000 ( 0x000[E-] ... 0x00F )
               [000003] I-C-G------                         *  CALL      struct System.RuntimeType:get_TypeHandle():System.RuntimeTypeHandle:this (exactContextHnd=0x00007FF7A7BA11C9)
               [000002] --C-G------ this                    \--*  CALL help ref    CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
               [000000] H---------- arg0                       \--*  CNS_INT(h) long   0x7ff7a7cfa2a8 class

***** BB01
STMT00001 ( 0x000[E-] ... ??? )
               [000006] -AC--------                         *  ASG       struct (copy)
               [000005] D------N---                         +--*  LCL_VAR   struct<System.RuntimeTypeHandle, 8> V01 loc0         
               [000004] --C--------                         \--*  RET_EXPR  struct(for [000003])

***** BB01
STMT00002 ( 0x010[E-] ... 0x017 )
               [000008] I-C-G------                         *  CALL      long   System.RuntimeTypeHandle:get_Value():long:this (exactContextHnd=0x00007FF7A7D21919)
               [000007] ----------- this                    \--*  LCL_ADDR  byref  V01 loc0         [+0]

***** BB01
STMT00003 ( 0x010[E-] ... ??? )
               [000010] --C--------                         *  RETURN    long  
               [000009] --C--------                         \--*  RET_EXPR  long  (for [000008])

image
(as a proof that it is not trivial to do anything with this in importer - we need:

  1. make sure the local is single-use
  2. find its def
  3. make sure the def is "before" use (which is possible)
    this sort of things we avoid doing in importer

We can sort of do this after importer and ForwardSub but we'll need to disable inlining for them which is not great either.

Copy link
Member

Choose a reason for hiding this comment

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

in this case we also will need some hacky way to work with a spilled struct RuntimeTypeHandle

If it helps, I would be ok with ignoring RuntimeTypeHandle.get_Value and tell people to use RuntimeTypeHandle.ToIntPtr for best results. We have added RuntimeTypeHandle.ToIntPtr/FromIntPtr family of APIs recently, as a managed equivalent of some Mono embedding APIs.

Copy link
Member Author

@EgorBo EgorBo May 7, 2023 •
edited

Choose a reason for hiding this comment

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

in this case we also will need some hacky way to work with a spilled struct RuntimeTypeHandle

If it helps, I would be ok with ignoring RuntimeTypeHandle.get_Value and tell people to use RuntimeTypeHandle.ToIntPtr for best results. We have added RuntimeTypeHandle.ToIntPtr/FromIntPtr family of APIs recently, as a managed equivalent of some Mono embedding APIs.

Yes, that one is at least do-able in importer, I've just added it

Copy link
Member

Choose a reason for hiding this comment

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

There is some special spill avoidance in the importer to make recognizing these kinds of multi-call patterns easier; you could extend this if needed.

runtime/src/coreclr/jit/importercalls.cpp

Lines 1414 to 1442 in 7d54c05

// For non-candidates we must also spill, since we
// might have locals live on the eval stack that this
// call can modify.
//
// Suppress this for certain well-known call targets
// that we know won't modify locals, eg calls that are
// recognized in gtCanOptimizeTypeEquality. Otherwise
// we may break key fragile pattern matches later on.
bool spillStack = true;
if (call->IsCall())
{
GenTreeCall* callNode = call->AsCall();
if ((callNode->gtCallType == CT_HELPER) && (gtIsTypeHandleToRuntimeTypeHelper(callNode) ||
gtIsTypeHandleToRuntimeTypeHandleHelper(callNode)))
{
spillStack = false;
}
else if ((callNode->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) != 0)
{
spillStack = false;
}
}
if (spillStack)
{
impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("non-inline candidate call"));
}
}
}

impAppendTree(asgHandleFld, CHECK_SPILL_NONE, impCurStmtDI);
retNode = impCreateLocalNode(structLcl DEBUGARG(0));
impPopStack();
}
break;
}

case NI_System_Type_get_IsEnum:
case NI_System_Type_get_IsValueType:
case NI_System_Type_get_IsByRefLike:
Expand Down Expand Up @@ -8109,6 +8154,10 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
{
result = NI_System_Type_get_IsEnum;
}
if (strcmp(methodName, "get_TypeHandle") == 0)
{
result = NI_System_RuntimeType_get_TypeHandle;
}
}
else if (strcmp(className, "RuntimeTypeHandle") == 0)
{
Expand Down Expand Up @@ -8206,6 +8255,10 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
{
result = NI_System_Type_op_Inequality;
}
else if (strcmp(methodName, "get_TypeHandle") == 0)
{
result = NI_System_Type_get_TypeHandle;
}
}
break;
}
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/namedintrinsiclist.h
View file
Open in desktop
Expand Up @@ -66,6 +66,7 @@ enum NamedIntrinsic : unsigned short
NI_System_Type_GetEnumUnderlyingType,
NI_System_Type_get_IsValueType,
NI_System_Type_get_IsByRefLike,
NI_System_Type_get_TypeHandle,
NI_System_Type_IsAssignableFrom,
NI_System_Type_IsAssignableTo,
NI_System_Type_op_Equality,
Expand All @@ -78,6 +79,7 @@ enum NamedIntrinsic : unsigned short
NI_System_Object_MemberwiseClone,
NI_System_Object_GetType,
NI_System_RuntimeTypeHandle_ToIntPtr,
NI_System_RuntimeType_get_TypeHandle,
NI_System_StubHelpers_GetStubContext,
NI_System_StubHelpers_NextCallReturnAddress,

Expand Down
20 changes: 17 additions & 3 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Expand Up @@ -12001,11 +12001,25 @@ bool CEEInfo::getObjectContent(CORINFO_OBJECT_HANDLE handle, uint8_t* buffer, in
_ASSERTE(objRef != NULL);

// TODO: support types containing GC pointers
if (!objRef->GetMethodTable()->ContainsPointers() && bufferSize + valueOffset <= (int)objRef->GetSize())
if (bufferSize + valueOffset <= (int)objRef->GetSize())
{
Object* obj = OBJECTREFToObject(objRef);
memcpy(buffer, (uint8_t*)obj + valueOffset, bufferSize);
result = true;
PTR_MethodTable type = obj->GetMethodTable();
if (type->ContainsPointers())
{
// RuntimeType has a gc field (object m_keepAlive), but if the object is in a frozen segment
// it means that field is always nullptr so we can read any part of the object:
if (type == g_pRuntimeTypeClass && GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(obj))
{
memcpy(buffer, (uint8_t*)obj + valueOffset, bufferSize);
result = true;
}
}
else
{
memcpy(buffer, (uint8_t*)obj + valueOffset, bufferSize);
result = true;
}
}

EE_TO_JIT_TRANSITION();
Expand Down
7 changes: 6 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs
View file
Open in desktop
Expand Up @@ -25,7 +25,12 @@ internal sealed partial class RuntimeType : TypeInfo, ICloneable
public override int MetadataToken => RuntimeTypeHandle.GetToken(this);
public override Module Module => GetRuntimeModule();
public override Type? ReflectedType => DeclaringType;
public override RuntimeTypeHandle TypeHandle => new RuntimeTypeHandle(this);
public override RuntimeTypeHandle TypeHandle
{
[Intrinsic] // to avoid round-trip "handle -> RuntimeType -> handle" in JIT
get => new RuntimeTypeHandle(this);
}

public override Type UnderlyingSystemType => this;

public object Clone() => this;
Expand Down
7 changes: 6 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/Type.cs
View file
Open in desktop
Expand Up @@ -424,7 +424,12 @@ public virtual MemberInfo GetMemberWithSameMetadataDefinitionAs(MemberInfo membe
| DynamicallyAccessedMemberTypes.PublicNestedTypes)]
public virtual MemberInfo[] GetDefaultMembers() => throw NotImplemented.ByDesign;

public virtual RuntimeTypeHandle TypeHandle => throw new NotSupportedException();
public virtual RuntimeTypeHandle TypeHandle
{
[Intrinsic]
get => throw new NotSupportedException();
}

public static RuntimeTypeHandle GetTypeHandle(object o)
{
ArgumentNullException.ThrowIfNull(o);
Expand Down